fix gflags bthread_concurrency_by_tag validate error

MJY-HUST commented 3 months ago

What problem does this PR solve?

Issue Number: https://github.com/apache/brpc/issues/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 commented 3 months ago

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

MJY-HUST commented 3 months ago

一般什么情况下会两次解析呀，我这边确实没遇到过这样的场景，这块就做的不充分。 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 commented 3 months ago

一般什么情况下会两次解析呀，我这边确实没遇到过这样的场景，这块就做的不充分。 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 commented 3 months ago

一般什么情况下会两次解析呀，我这边确实没遇到过这样的场景，这块就做的不充分。 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 commented 3 months ago

LGTM

wwbmmm commented 3 months ago

LGTM

apache / brpc