Skip to content

fix(rpc_replay) continue when failed to init channel #1938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
wwbmmm merged 5 commits into from
Oct 8, 2022
Merged

fix(rpc_replay) continue when failed to init channel #1938

wwbmmm merged 5 commits into from
Oct 8, 2022

Conversation

@ehds
Copy link
Contributor

@ehds ehds commented Sep 29, 2022 •
edited
Loading

rpc_replay 在 ChannelGroup 初始化时，即使有channel Init 失败（预期行为）应该直接 continue， 而不是返回 -1, 否则会导致程序直接退出，无法继续向下执行。

例如当 -connection_type=pooled， rtmp 协议由于不支持 pooled 模式，导致程序退出。本来的 rpc_data 中只有 baidu_std 协议，导致无法正常使用。

@serverglen
Copy link
Contributor

serverglen commented Sep 30, 2022

LGTM

wwbmmm
wwbmmm reviewed Sep 30, 2022
View reviewed changes
tools/rpc_replay/rpc_replay.cpp Outdated
&options) != 0) {
LOG(ERROR) << "Fail to initialize channel";
return -1;
LOG(WARNING) << "Fail to initialize channel";
Copy link
Contributor

@wwbmmm wwbmmm Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

能否先比较protocols[i].second.supported_connection_type和FLAGS_connection_type，遇到不支持的直接跳过呢。
这里Channel::Init失败可能有其它原因，比如FLAGS_server或FLAGS_load_balancer配错，这种情况如果继续往下走就不合适了

Copy link
Contributor Author

@ehds ehds Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

wwbmmm
wwbmmm reviewed Sep 30, 2022
View reviewed changes
tools/rpc_replay/rpc_replay.cpp Outdated
if (protocols[i].second.support_client() &&
protocols[i].second.support_server()) {
protocols[i].second.support_server() &&
(brpc::StringToConnectionType(FLAGS_connection_type) &
Copy link
Contributor

@wwbmmm wwbmmm Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brpc::StringToConnectionType可能返回CONNECTION_TYPE_UNKNOWN，需要处理下这种情况

Copy link
Contributor Author

@ehds ehds Oct 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

目前是把 ChannelOptions 的初始化放到判断之前，方便用于判断 connection_type 和 protocol 的支持关系。如果connection_type 为 CONNECTION_TYPE_UNKNOWN 或者 protocol 支持用户指定的 connection_type 才继续往下。这里没有判断 connection_type 是否 has_error(用户输入非法的 connection_type），因为 InitChannelOptions 中会根据协议支持的 protoctol connection_type 进行选择。

@wwbmmm wwbmmm merged commit 5a6d99a into apache:master Oct 8, 2022
@Huixxi Huixxi added the bug the code does not work as expected label Oct 10, 2022
guodongxiaren added a commit to guodongxiaren/incubator-brpc that referenced this pull request Nov 21, 2022
@guodongxiaren @Tuvie
@wwbmmm @bumingchun @acelyc111 @zyearn @hcoona @yyweii @tobegit3hub @opheliaKyouko @morningman @JackBoosY @tanzhongyi003 @TousakaRin @dabao085 @Huixxi @tullyliu @day253 @ehds @fansehep @serverglen @adonis0147 @cuishuang @wasphin @lorinlee
Linux workflow (#14)
6c196fb 
* enable brpc use rdma

* Fix override issue in pb 3.21

* fix rpc_replay can't send request equably (apache#1910)

* fix rpc_replay can't send request equably

* 类型修改

* fix coredump cause by uri like 'host:port/hotspots/growth_non_responsive?console=abc' (apache#1278)

* expose logging::PrintLog

* Support -escape_log

* Fix thrift_message pb override issue

* Chore: rework Bazel build system

* remove white space from default value of bvar_dump_tabs

* Update oncall.md

* Fix not to abort when checking the errorno with unicode string (apache#1142)

* fix a typo in grpc protocol (apache#1924)

* fix a typo in grpc protocol

* ERESPONSE->EREQUEST

Co-authored-by: 薛传宇 <xuechuanyu@cmss.chinamobile.com>

* [user-cases] add Apache Doris user case

* add nacos naming service (apache#1922)

* [document] Add vcpkg instruction step (apache#1925)

* http response uses brpc error code (apache#1927)

* http response uses brpc error code

* add gflag for using http error code

* add unit test of http error code

* Update Oncall record

* Fix bvar compile error (apache#1937)

* Fix bug butex_wait failed with timeout (apache#1917)

* Fix bug butex_wait failed with timeout

Co-authored-by: XiguoHu <huxiguo@baidu.com>

* fix issues in FlatMap

* Update release_cn.md

* brpc在BaikalDB中的应用

* Update getting_started.md

* fix(rpc_replay) continue when failed to init channel (apache#1938)

* fix(rpc_replay) continue when failed to init channel

* check supported_connection_type

* check supported_connection_type

* check supported_connection_type

* fix lint

* Update cases.md (apache#1944)

* fix rpc_press.md (apache#1942)

Signed-off-by: fan <yfan3763@gmail.com>

Signed-off-by: fan <yfan3763@gmail.com>

* fix typo in json2pb doc (apache#1939)

* Update oncall.md (apache#1949)

* Update release_cn.md

* Update RELEASE_VERSION

* Update CMakeLists.txt

* Update brpc.spec

* Update release_cn.md

* Update release_cn.md

* Update release_cn.md

* add pull_request_template.md (apache#1952)

Signed-off-by: fan <yfan3763@gmail.com>

Signed-off-by: fan <yfan3763@gmail.com>

* Fix the linkage errors caused by duplicate symbols (apache#1936)

* Fix "sched_to itself" error when buidling by Clang on Linux aarch64 (apache#1950)

* docs: fix some typos

Signed-off-by: cui fliter <imcusg@gmail.com>

* Fix source file mode

* rpm: support RHEL9

* Update oncall.md

* Update newcommitter.md

* fix arena cleared early when parse redis message

* community: Update oncall.md (apache#1960)

Co-authored-by: lei.li <lei.li@clickzetta.com>

* Reduce UT log output

* Update release_cn.md

* Update release_cn.md

Update brpc's brief introduction in Announce mail.

* Update release_cn.md

* Macos workflow (#10)

* fix typo

* delete bazel from mac workflow

* fix exceptation value for mac ut

* delete test

* Create ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

* Update ci_linux.yml

Signed-off-by: fan <yfan3763@gmail.com>
Signed-off-by: cui fliter <imcusg@gmail.com>
Co-authored-by: Tuvie <lizhaogeng1989@gmail.com>
Co-authored-by: wwbmmm <wwbmmm@163.com>
Co-authored-by: bumingchun <bumingchun@126.com>
Co-authored-by: Yingchun Lai <acelyc1112009@gmail.com>
Co-authored-by: gejun.0 <gejun.0@bytedance.com>
Co-authored-by: Jiashun Zhu <zhujiashun2010@gmail.com>
Co-authored-by: Shuai Zhang <zhangshuai.ustc@gmail.com>
Co-authored-by: yyweii <thymene@gmail.com>
Co-authored-by: tobe <tobeg3oogle@gmail.com>
Co-authored-by: bbbezxcy <bbezxcy@qq.com>
Co-authored-by: 薛传宇 <xuechuanyu@cmss.chinamobile.com>
Co-authored-by: morningman <morningman@163.com>
Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>
Co-authored-by: Tanzhongyi(Jerry Tan) <jerrytan@apache.org>
Co-authored-by: chenBright <chenguangming@bigo.sg>
Co-authored-by: lei he <lhestz@163.com>
Co-authored-by: Chengx <chengxiang085@gmail.com>
Co-authored-by: HU <uestc.hugo@gmail.com>
Co-authored-by: XiguoHu <huxiguo@baidu.com>
Co-authored-by: Tao Liu <liutao04@baidu.com>
Co-authored-by: day253 <9634619+day253@users.noreply.github.com>
Co-authored-by: ds <ehds@qq.com>
Co-authored-by: fan <75058860+fansehep@users.noreply.github.com>
Co-authored-by: serverglen <serverglen@gmail.com>
Co-authored-by: Adonis Ling <adonis0147@gmail.com>
Co-authored-by: cui fliter <imcusg@gmail.com>
Co-authored-by: Xiaofeng Wang <wasphin@gmail.com>
Co-authored-by: jiumei <jiumei@xiaohongshu.com>
Co-authored-by: LorinLee <lorinlee1996@gmail.com>
Co-authored-by: lei.li <lei.li@clickzetta.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wwbmmm wwbmmm wwbmmm left review comments

Assignees

No one assigned

Labels

bug the code does not work as expected

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ehds @serverglen @wwbmmm @Huixxi