Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10342 )
Change subject: [flags] run validators after processing help flags ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/cfile/block_cache.cc@45 PS7, Line 45: // For details, see https://gerrit.cloudera.org/#/c/10342/ > You can omit this; it'll be in the git commit message anyway. I think it's worth keeping the comment about the default value set to 'true', at least to keep readers of the code less confused. Indeed, adding gerrit review item information was redundant; it's removed. http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/master/master_main.cc File src/kudu/master/master_main.cc: http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/master/master_main.cc@58 PS7, Line 58: to > omit Done http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/master/master_main.cc@60 PS7, Line 60: SetCommandLineOptionWithMode("force_block_cache_capacity", "false", > Check that the return value is not empty. I was following the most common notation of using this method in the Kudu code; indeed, that's not safe. http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/tools/CMakeLists.txt File src/kudu/tools/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/tools/CMakeLists.txt@18 PS7, Line 18: set(LINK_LIBS > This is only used in kudu_tools_util, so maybe just merge it in there? Done http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/tools/CMakeLists.txt@61 PS7, Line 61: server_process > Sort order. Done http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/tserver/tablet_server_main.cc File src/kudu/tserver/tablet_server_main.cc: http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/tserver/tablet_server_main.cc@63 PS7, Line 63: to > omit Done http://gerrit.cloudera.org:8080/#/c/10342/7/src/kudu/tserver/tablet_server_main.cc@65 PS7, Line 65: SetCommandLineOptionWithMode("force_block_cache_capacity", "false", > Check return value. Done -- To view, visit http://gerrit.cloudera.org:8080/10342 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I51f263890c91251f0d13c826c40dfd20fe284b59 Gerrit-Change-Number: 10342 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Thu, 10 May 2018 17:21:01 +0000 Gerrit-HasComments: Yes
