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 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19237/1//COMMIT_MSG@15 PS1, Line 15: : Therefore this > How about adding another parameter: --skip_check_consistency to skip to che Yes, I think it's a good idea to add an extra flag to enable/disable checking for flag consistency in SetFlag() RPC. If that looks like a preferable solution to you, I'm totally fine with that :) The only thing I'd ask is to name the newly introduced flag with positive, not negative semantics: i.e. call it something like --check_for_flags_consistency (or just --check_flags_consistency), and have it 'true' by default. If you choose to use the route with a new RPC, as for the complications about a new RPC to set flags in groups: yes, I think we should try to set all the flags which come with a request and roll them back altogether. And I wouldn't bother too much about various misuse scenarios once the implementation is safe to rollback the previous snapshot of the flags which is consistent. I think that the former solution is simpler and I like it better (i.e. I'd vote for the solution with the extra flag than introducing a new RPC). http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@14 PS4, Line 14: unvailable unavailable http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@18 PS4, Line 18: consitency consistency http://gerrit.cloudera.org:8080/#/c/19237/4//COMMIT_MSG@26 PS4, Line 26: shoud should http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@156 PS4, Line 156: configs nit: flags http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@162 PS4, Line 162: Roll back nit: rolled back Also, consider using Substitute() to compose the message -- it's easier to read in the code http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/generic_service.cc@171 PS4, Line 171: { nit: adding this extra scope isn't necessary, so maybe remove the curly braces at lines 171 and 178? http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto File src/kudu/server/server_base.proto: http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@80 PS4, Line 80: consistency for consistency http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@80 PS4, Line 80: set a flag. ... the flag is set. http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/server/server_base.proto@81 PS4, Line 81: skip_check_consistency I think it makes sense to rename this to have a 'positive' semantics instead (e.g., 'run_consistency_check' instead of 'skip_consistency_check') and keep it false by default in this proto file. The reason behind keeping it 'false' by default in the prot file is to maintain backwards compatibility for older clients (i.e. older binaries or kudu CLI tools) which aren't able to disable the consistency check otherwise. However, the flag should be set 'true' by default when sending a request to a server in the implementation of the update 'kudu <master|tserver> set_flag' CLI tools. Does it make sense? http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/util/flags.cc File src/kudu/util/flags.cc: http://gerrit.cloudera.org:8080/#/c/19237/4/src/kudu/util/flags.cc@444 PS4, Line 444: LOG(ERROR) << " Check validtor: " << e.first << " failed."; I guess this isn't needed since every validator prints detailed message on what was wrong. Also, it seems we might need different methods for validating the flags during SetFlag() and while running those checks upon starting up a process. For the validator targeted for SetFlag(), it makes sense to return at the first report on the flag inconsistency. However, for the validator that's run during starting up a process, it makes sense to print out all the issues detected, so it's easier to address everything right away before trying to start the process again. So, there the approach used in former revision of RunCustomValidators() should be used (e.g. something like 'found_inconsistency |= !e.second()' and continuing with the loop). -- 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: 4 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Tue, 15 Nov 2022 03:11:38 +0000 Gerrit-HasComments: Yes
