Skip to content
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.

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

fix gflags bthread_concurrency_by_tag validate error #2730

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

MJY-HUST
Copy link
Contributor

@MJY-HUST MJY-HUST commented Aug 8, 2024

What problem does this PR solve?

Issue Number:
#2542

Problem Summary:
升级到1.10.0版本后,重复解析两次 gflags 后,flags bthread_concurrency_by_tag validate会报错的问题又复现了。
是由于在默认配置下,第二次解析gflags时,num = 0,tag_ngroup = FALGS_bthread_concurrency。两者不想等导致返回了EPERM。
gflags的默认配置下,只有一个tag且等于0,bthread_concurrency_by_tag的默认值应该设置成等于bthread_concurrency的默认值,这样也比较合理。
6M4`0}5K%XVK@V@0(DNK71K

What is changed and the side effects?

Changed:

Side effects:

  • Performance effects(性能影响):

  • Breaking backward compatibility(向后兼容性):


Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

@yanglimingcn
Copy link
Contributor

yanglimingcn commented Aug 9, 2024

一般什么情况下会两次解析呀,我这边确实没遇到过这样的场景,这块就做的不充分。
bthread_concurrency_by_tag的默认值应该设置成等于bthread_concurrency的默认值,如果有多个tag的时候,两次解析是啥结果呢?

@MJY-HUST
Copy link
Contributor Author

MJY-HUST commented Aug 9, 2024

一般什么情况下会两次解析呀,我这边确实没遇到过这样的场景,这块就做的不充分。 bthread_concurrency_by_tag的默认值应该设置成等于bthread_concurrency的默认值

两次解析是由于上层应用对其他的gflags有reload需求,但使用了类似ParseCommandLineFlags会调用所有flags的validate函数?或者只是代码的不规范导致的在链路中执行了两次ParseCommandLineFlags触发这个问题。这部分应该是可以通过修改上层应用的代码来避免,但是brpc应该也需要做到兼容这个场景。

如果有多个tag的时候,两次解析是啥结果呢?

如果内部实现有多个tag,一般来说第一次解析是在main函数开始出,此时第一次解析由于设置了never_set_bthread_concurrency_by_tag,task_control还是等于nullptr;后续如果在设置了tag的server没启动前出现第二次解析,此时task_control init之后只有tag 0,因此指定其他的tag时,tag_ngroup = 0,如果设置了bthread_concurrency_by_tag的值,那么只要其大于0,那么就会触发add_workers,不会有异常。如果在server启动之后第二次触发,就和正常的动态改变分组线程的流程一致了,应该也没有问题。

@yanglimingcn
Copy link
Contributor

yanglimingcn commented Aug 9, 2024

一般什么情况下会两次解析呀,我这边确实没遇到过这样的场景,这块就做的不充分。 bthread_concurrency_by_tag的默认值应该设置成等于bthread_concurrency的默认值

两次解析是由于上层应用对其他的gflags有reload需求,但使用了类似ParseCommandLineFlags会调用所有flags的validate函数?或者只是代码的不规范导致的在链路中执行了两次ParseCommandLineFlags触发这个问题。这部分应该是可以通过修改上层应用的代码来避免,但是brpc应该也需要做到兼容这个场景。

如果有多个tag的时候,两次解析是啥结果呢?

如果内部实现有多个tag,一般来说第一次解析是在main函数开始出,此时第一次解析由于设置了never_set_bthread_concurrency_by_tag,task_control还是等于nullptr;后续如果在设置了tag的server没启动前出现第二次解析,此时task_control init之后只有tag 0,因此指定其他的tag时,tag_ngroup = 0,如果设置了bthread_concurrency_by_tag的值,那么只要其大于0,那么就会触发add_workers,不会有异常。如果在server启动之后第二次触发,就和正常的动态改变分组线程的流程一致了,应该也没有问题。

我觉得得测试一下这些场景,如果设置了task_group_ntags=2,那么在task_control init的时候会把FLAGS_bthread_concurrency平均分配给每组。这时候再执行bthread_setconcurrency_by_tag把tag0的worker数量改变了,这个线程数量是你想要的吗?感觉这块考虑的是不是过于复杂了。
可以把FLAGS_bthread_current_tag的默认值变成一个非法值BTHREAD_TAG_INVALID,然后,bthread_setconcurrency_by_tag(val, FLAGS_bthread_current_tag) 这个函数里面判断FLAGS_bthread_current_tag 是非法值,就直接返回成功,相当于没啥效果。把never_set_bthread_concurrency_by_tag这个就去掉了。

@MJY-HUST
Copy link
Contributor Author

一般什么情况下会两次解析呀,我这边确实没遇到过这样的场景,这块就做的不充分。 bthread_concurrency_by_tag的默认值应该设置成等于bthread_concurrency的默认值

两次解析是由于上层应用对其他的gflags有reload需求,但使用了类似ParseCommandLineFlags会调用所有flags的validate函数?或者只是代码的不规范导致的在链路中执行了两次ParseCommandLineFlags触发这个问题。这部分应该是可以通过修改上层应用的代码来避免,但是brpc应该也需要做到兼容这个场景。

如果有多个tag的时候,两次解析是啥结果呢?

如果内部实现有多个tag,一般来说第一次解析是在main函数开始出,此时第一次解析由于设置了never_set_bthread_concurrency_by_tag,task_control还是等于nullptr;后续如果在设置了tag的server没启动前出现第二次解析,此时task_control init之后只有tag 0,因此指定其他的tag时,tag_ngroup = 0,如果设置了bthread_concurrency_by_tag的值,那么只要其大于0,那么就会触发add_workers,不会有异常。如果在server启动之后第二次触发,就和正常的动态改变分组线程的流程一致了,应该也没有问题。

我觉得得测试一下这些场景,如果设置了task_group_ntags=2,那么在task_control init的时候会把FLAGS_bthread_concurrency平均分配给每组。这时候再执行bthread_setconcurrency_by_tag把tag0的worker数量改变了,这个线程数量是你想要的吗?感觉这块考虑的是不是过于复杂了。 可以把FLAGS_bthread_current_tag的默认值变成一个非法值BTHREAD_TAG_INVALID,然后,bthread_setconcurrency_by_tag(val, FLAGS_bthread_current_tag) 这个函数里面判断FLAGS_bthread_current_tag 是非法值,就直接返回成功,相当于没啥效果。把never_set_bthread_concurrency_by_tag这个就去掉了。

是的,默认设置为BTHREAD_TAG_INVALID更合理,按照你的意见修改了

@yanglimingcn
Copy link
Contributor

LGTM

1 similar comment
@wwbmmm
Copy link
Contributor

wwbmmm commented Aug 19, 2024

LGTM

@yanglimingcn yanglimingcn self-requested a review August 22, 2024 02:25
@yanglimingcn yanglimingcn merged commit d3c6854 into apache:master Aug 27, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants