Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 )
Change subject: [tools] ksck improvements [6/n]: Refactor result handling ...................................................................... Patch Set 12: (9 comments) http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@a281 PS12, Line 281: : : > Is this still no longer needed? Done http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@a994 PS12, Line 994: > This too? This is redundant- if FLAGS_consensus = false, then the ContainsKey on L801 of PS12 will return false and no consensus state will be recorded. http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@a1024 PS12, Line 1024: > This too? Same, if FLAGS_consensus = false, r.consensus_state will be boost::none. http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@194 PS12, Line 194: return master->FetchConsensusState(); : }); > nit: not 100% sure how to interpret the GSG wrt lambdas (I would've thought I think this is actually consistent with the GSG, since this lambda is an argument to a function and arguments get indented 4. There's then 2 more spaces for regular indentation inside a block. I'm not particular on it looking a certain way if there's rough consistency and it's readable. I'll make 286 consistent with this. Languages need tools that do the formatting so we can't stop worrying about stuff like this. http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@284 PS12, Line 284: VLOG(1) << "Going to connect to tablet server: " << ts->uuid(); > nit: same here wrt spacing. Maybe I'm missing something but six spaces seem I agree it's a lot but I think 4 argument continuation + 2 for inside block is right. Imagine if the lambda were on a line of its own-- then the 4 would definitely be right, right? http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@297 PS12, Line 297: const char* const wrong_uuid_msg = "ID reported by tablet server " : "doesn't match the expected ID"; > Do we have test cases that would fail if this message were to change? It's worth moving this to a separate patch. http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@300 PS12, Line 300: > nit: strange place for a space ? Done http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck_results.cc@182 PS12, Line 182: || master_consensus_conflict > This should also be gated by FLAGS_consensus, right? Or in PrintConsensusMa It'll always be false if there's no consensus info. http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/tool_action_cluster.cc@a45 PS12, Line 45: : : > This probably belongs in the commit message too (or in a separate patch). I It just moved to ksck.cc. -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 12 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 30 Apr 2018 19:48:48 +0000 Gerrit-HasComments: Yes
