Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19237 )
Change subject: [Flag] Check flags consistency when setting a flag ...................................................................... Patch Set 3: (13 comments) > Patch Set 1: > > (2 comments) http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG Commit Message: PS1: > Whoops, sorry -- I see there are some test scenarios in kudu-tool-test.cc a Done http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@9 PS1, Line 9: There are existing some dependencies between some flags. : For example, if sett > This sounds a bit confusing because it might be attributed to the case when Done http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@10 PS1, Line 10: ng auto_rebalancing_enabled to true, : unlock_experimental_flags must be s > I'm not sure I understand what you meant by that. The flags set via Generi Done http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@15 PS1, Line 15: : Therefore this > One issue with this approach I can see is that once this consistency check How about adding another parameter: --skip_check_consistency to skip to check all flags consistency? Then the user can set a group flags one by one. If user use the parameter skip_check_consistency, they know the risk. Define a function to set multiple flags at once can solve the problem: set a group flags. But users may be use it to set multiple flags which are not in a flag group. At that case, shall we also check the consistency? If so, one flag set failed, shall we roll back all flags? http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@17 PS1, Line 17: t > Should this be 'and'? I'm not sure this makes any sense as it's written no Done 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 Done http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/server/generic_service.cc@174 PS1, Line 174: > nit: this 'return' isn't needed Done 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 fo Done 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: Check unsafe flags, experimental flags, flag group valid > The description should include the essential information on the returned va Done http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.h@61 PS1, Line 61: true, the system is in > The semantics of this method is a bit confusing to me -- it returns 'true' Done 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' Done http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@444 PS1, Line 444: LOG(ERROR) < > The original code runs all the validators, and that was intentional for Run Done http://gerrit.cloudera.org:8080/#/c/19237/1/src/kudu/util/flags.cc@551 PS1, Line 551: > After some consideration, I realized the standard validators defined by DEF Done -- 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: 3 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Comment-Date: Mon, 14 Nov 2022 12:14:38 +0000 Gerrit-HasComments: Yes
