Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12517 )
Change subject: [tools] Support running the master and tablet server via the kudu binary ...................................................................... Patch Set 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/master/master_runner.cc File src/kudu/master/master_runner.cc: http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/master/master_runner.cc@68 PS11, Line 68: Explicitly set the flag along with the default as this should not be overridden. I'm not sure I understand this comment. Does it say that the value for the flag hasn't been set yet, so it's necessary to set the value directly? My confusion comes from the code in src/gflags.cc in gflags: from FlagRegistry::SetFlagLocked() I can see the value itself would be by calling SetCommandLineOptionWithMode(..., ..., SET_FLAGS_DEFAULT) if the flags hasn't been modified yet. Maybe, update the comment to say: Explicitly set the flag to the current value of the flag in case if SetCommandLineOptionWithMode() didnt' touch it. SetCommandLineOptionWithMode() leaves the current value unchanged if it has been modified prior to the call. http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/tserver/tablet_server_runner.cc File src/kudu/tserver/tablet_server_runner.cc: http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/tserver/tablet_server_runner.cc@64 PS11, Line 64: CHECK_NE("", SetCommandLineOptionWithMode("force_block_cache_capacity", : "false", gflags::SET_FLAGS_DEFAULT)); nit: do you want to set the force_block_cache_capacity flag explicitly to the desired value as well? I think relying on the fact that the flag hasn't been changed yet is brittle once this call is moved into the SetTabletServerFlagDefaults() function. The newly introduced function might be called after the 'force_block_cache_capacity' flag is set to true somewhere else, right? http://gerrit.cloudera.org:8080/#/c/12517/11/src/kudu/util/rolling_log.h File src/kudu/util/rolling_log.h: PS11: maybe, put this update and the places where it's used (diagnostics_log.cc, server_base.cc) into a separate changelist? -- To view, visit http://gerrit.cloudera.org:8080/12517 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e Gerrit-Change-Number: 12517 Gerrit-PatchSet: 11 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 20 Feb 2019 23:16:52 +0000 Gerrit-HasComments: Yes
