Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG Commit Message: PS1: > Tests are missing. Please add tests to cover the newly added functionality Whoops, sorry -- I see there are some test scenarios in kudu-tool-test.cc actually. Please disregard this particular comment. http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc@158 PS1, Line 158: string res = google::SetCommandLineOption( What if rolling back the flag failed? Should the server at least output a warning about that into the logs? http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc@174 PS1, Line 174: return; nit: this 'return' isn't needed http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/tools/kudu-tool-test.cc@4366 PS1, Line 4366: TEST_F(ToolTest, TestServerSetFlag) { Could you also add a scenario that tries to set valid and invalid values for a flag that has a validator defined via DEFINE_validator(), e.g. for --rpc_tls_min_protocol? http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h File src/kudu/util/flags.h: http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h@60 PS1, Line 60: Verifies that the flags are allowed to be set and valid. The description should include the essential information on the returned values. http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h@61 PS1, Line 61: CheckFlagsInconsistence The semantics of this method is a bit confusing to me -- it returns 'true' if there is a problem, and 'false' otherwise. Please rename this method to follow the style guideline and make fix its semantics per https://google.github.io/styleguide/cppguide.html#General_Naming_Rules I'd suggest naming this AreFlagsConsistent() and return 'false' if an inconsistency is detected, 'true' otherwise. http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc File src/kudu/util/flags.cc: http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@443 PS1, Line 443: if(!e.second()) { nit: missing space after 'if' http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@444 PS1, Line 444: return true; The original code runs all the validators, and that was intentional for RunCustomValidators() -- to output information on all the detected inconsistencies at once. http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@551 PS1, Line 551: bool CheckFlagsInconsistence() { This current code misses running the regular (i.e. non-custom) validators for the gflags. In other words, the bug here is that the validators installed via DEFINE_validator() aren't run, and non-acceptable values for those flags might still be set. -- To view, visit http://gerrit.cloudera.org:8080/19237 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e6e37e0f4e72a2cc3d2316248961315f1757ebc Gerrit-Change-Number: 19237 Gerrit-PatchSet: 1 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 12 Nov 2022 19:35:41 +0000 Gerrit-HasComments: Yes
