Andrew Wong 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: (11 comments) http://gerrit.cloudera.org:8080/#/c/10151/11/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10151/11/src/kudu/tools/ksck.cc@297 PS11, Line 297: t wrong_uuid_msg = "ID reported by tablet server " : "doesn't match the expected ID"; > :/ I hacked in a more precise but more brittle check. I'm not sure we can s Hrmm.. Thanks for making this change, sorry I'm gonna be waffle here and say I think this sort of thing probably also belongs in a separate patch too, since it could probably use a test too. Maybe keep this as it was, open a JIRA, and point this at it? Or add a test in this patch and scope creep a little bit (including a point in the commit message) 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? http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@a994 PS12, Line 994: This too? http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@a1024 PS12, Line 1024: This too? 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 it'd be four spaces to the left), so I'll just point at this and look to you for confirmation that this is how you want it to be spaced. See L286, I was expecting something more like that. 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 seems like a lot? 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? http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@300 PS12, Line 300: nit: strange place for a space ? 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 PrintConsensusMatrix() maybe http://gerrit.cloudera.org:8080/#/c/10151/11/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/10151/11/src/kudu/tools/tool_action_cluster.cc@a146 PS11, Line 146: > Done Hmm don't see this in PS12 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). Is this completely gone? -- 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: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 30 Apr 2018 18:45:15 +0000 Gerrit-HasComments: Yes
