[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10151 ) Change subject: [tools] ksck improvements [6/n]: Refactor result handling .. [tools] ksck improvements [6/n]: Refactor result handling This patch refactors ksck so the results of the checks are collected in a new KsckResults struct. Printing results is now a matter of iterating over the KsckResults struct and formatting the information within it. Furthermore, the KsckResults struct also serves as a programmatic access point to the results of ksck, which can be used for other things like rebalancing, auto-repair, or machine-readable printing. There's also a couple of bonus changes: - Added Ksck::Run and Ksck::RunAndPrintResults methods, which simplify running all ksck checks and printing the results, as is done in the ksck CLI tool and in ClusterVerifier. - Add the changes in https://gerrit.cloudera.org/#/c/10054/, which were about to be committed but would've needed to be redone a bit to fit with this refactor. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Reviewed-on: http://gerrit.cloudera.org:8080/10151 Reviewed-by: Andrew Wong Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_remote-test.cc A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 10 files changed, 1,195 insertions(+), 879 deletions(-) Approvals: Andrew Wong: Looks good to me, approved Kudu Jenkins: Verified -- 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: merged Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 15 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
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 14: Code-Review+2 -- 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: 14 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 01 May 2018 16:40:35 + Gerrit-HasComments: No
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
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 11: (1 comment) 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: > I think you missed this? Done -- 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: 11 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 01 May 2018 16:37:03 + Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10151 to look at the new patch set (#14). Change subject: [tools] ksck improvements [6/n]: Refactor result handling .. [tools] ksck improvements [6/n]: Refactor result handling This patch refactors ksck so the results of the checks are collected in a new KsckResults struct. Printing results is now a matter of iterating over the KsckResults struct and formatting the information within it. Furthermore, the KsckResults struct also serves as a programmatic access point to the results of ksck, which can be used for other things like rebalancing, auto-repair, or machine-readable printing. There's also a couple of bonus changes: - Added Ksck::Run and Ksck::RunAndPrintResults methods, which simplify running all ksck checks and printing the results, as is done in the ksck CLI tool and in ClusterVerifier. - Add the changes in https://gerrit.cloudera.org/#/c/10054/, which were about to be committed but would've needed to be redone a bit to fit with this refactor. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 --- M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_remote-test.cc A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 10 files changed, 1,195 insertions(+), 879 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/14 -- 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: newpatchset Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 14 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
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 13:
(4 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@a994
PS12, Line 994:
> This is redundant- if FLAGS_consensus = false, then the ContainsKey on L801
Gotcha, thanks for the clarification!
http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@194
PS12, Line 194: return master->FetchConsensusState();
: });
> I think this is actually consistent with the GSG, since this lambda is an a
Yeah, I agree it's readable so I'm fine with it, so long as it's consistent
within the PS.
http://gerrit.cloudera.org:8080/#/c/10151/12/src/kudu/tools/ksck.cc@284
PS12, Line 284: r (const auto& entry : cluster_->tablet_servers()) {
> I agree it's a lot but I think 4 argument continuation + 2 for inside block
Yeah, I can buy that.
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:
> Hmm don't see this in PS12
I think you missed this?
--
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: 13
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Mon, 30 Apr 2018 21:29:05 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
Hello Alexey Serbin, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10151 to look at the new patch set (#13). Change subject: [tools] ksck improvements [6/n]: Refactor result handling .. [tools] ksck improvements [6/n]: Refactor result handling This patch refactors ksck so the results of the checks are collected in a new KsckResults struct. Printing results is now a matter of iterating over the KsckResults struct and formatting the information within it. Furthermore, the KsckResults struct also serves as a programmatic access point to the results of ksck, which can be used for other things like rebalancing, auto-repair, or machine-readable printing. There's also a couple of bonus changes: - Added Ksck::Run and Ksck::RunAndPrintResults methods, which simplify running all ksck checks and printing the results, as is done in the ksck CLI tool and in ClusterVerifier. - Add the changes in https://gerrit.cloudera.org/#/c/10054/, which were about to be committed but would've needed to be redone a bit to fit with this refactor. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 --- M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_remote-test.cc A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 10 files changed, 1,195 insertions(+), 880 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/13 -- 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: newpatchset Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 13 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 30 Apr 2018 19:48:48 + Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 30 Apr 2018 18:45:15 + Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
Alexey Serbin 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: Code-Review+2 (1 comment) LGTM, maybe you need to get more feedback from Andrew. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.h@404 PS5, Line 404: std::ostream* out = nullptr > We still use it internally for checksum stuff progress updates-- it's more sgtm -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 28 Apr 2018 02:24:16 + Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
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: Verified+1 -- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 28 Apr 2018 02:16:39 + Gerrit-HasComments: No
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
Will Berkeley has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10151 ) Change subject: [tools] ksck improvements [6/n]: Refactor result handling .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- 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: deleteReviewer Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 12 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
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 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/10151/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10151/11//COMMIT_MSG@20 PS11, Line 20: about to be committed but would've needed to be redone a bit to fit : with this refactor. : > Just checking, is this the only user-facing change, other than some minor s Yes. http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@674 PS9, Line 674: ASSERT_STR_CONTAINS > Ah! Seems you missed this one and others similar. :( 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: : } > Not your fault, but I don't think this is always the case. I just tried run :/ I hacked in a more precise but more brittle check. I'm not sure we can so better without returning more information from the fetching, which would be some work to integrate. http://gerrit.cloudera.org:8080/#/c/10151/11/src/kudu/tools/ksck.cc@340 PS11, Line 340: PUSH_PREPEND_NOT_OK(ChecksumData(ChecksumOptions()), : results_.error_messages, "checksum scan error"); : > Why isn't this a prereq for the below checks? Because tablet servers may fail to give us info and we can still do checks. If we fail to get info from the leader master we are donezo. 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: > Now that this patch is keeping `--consensus`, this probably isn't needed? Done -- 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: 9 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 28 Apr 2018 00:58:47 + Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10151 to look at the new patch set (#12). Change subject: [tools] ksck improvements [6/n]: Refactor result handling .. [tools] ksck improvements [6/n]: Refactor result handling This patch refactors ksck so the results of the checks are collected in a new KsckResults struct. Printing results is now a matter of iterating over the KsckResults struct and formatting the information within it. Furthermore, the KsckResults struct also serves as a programmatic access point to the results of ksck, which can be used for other things like rebalancing, auto-repair, or machine-readable printing. There's also a couple of bonus changes: - Added Ksck::Run and Ksck::RunAndPrintResults methods, which simplify running all ksck checks and printing the results, as is done in the ksck CLI tool and in ClusterVerifier. - Add the changes in https://gerrit.cloudera.org/#/c/10054/, which were about to be committed but would've needed to be redone a bit to fit with this refactor. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 --- M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_remote-test.cc A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 10 files changed, 1,197 insertions(+), 886 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/12 -- 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: newpatchset Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 12 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
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 11: (5 comments) http://gerrit.cloudera.org:8080/#/c/10151/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10151/11//COMMIT_MSG@20 PS11, Line 20: - Add the changes in https://gerrit.cloudera.org/#/c/10054/, which were : about to be committed but would've needed to be redone a bit to fit : with this refactor. Just checking, is this the only user-facing change, other than some minor syntax? http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@674 PS9, Line 674: > Done Ah! Seems you missed this one and others similar. 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: s.IsRemoteError() ? : summary.health = KsckServerHealth::WRONG_SERVER_UUID : Not your fault, but I don't think this is always the case. I just tried running a relatively recent ksck on a cluster and got the following remote errors for all tablet servers: WARNING: Errors gathering consensus info for Tablet Server 70f7ee61ead54b1885d819f354eb3405 (vc1316.halxg.cloudera.com:7050): Remote error: could not fetch all consensus info: Not authorized: unauthorized access to method: GetConsensusState And so all of the tservers incorrectly reported WRONG_SERVER_UUID. http://gerrit.cloudera.org:8080/#/c/10151/11/src/kudu/tools/ksck.cc@340 PS11, Line 340: PUSH_PREPEND_NOT_OK(FetchInfoFromTabletServers(), results_.error_messages, : "error fetching info from tablet servers"); : Why isn't this a prereq for the below checks? 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: Now that this patch is keeping `--consensus`, this probably isn't needed? -- 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: 11 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 27 Apr 2018 18:47:25 + Gerrit-HasComments: Yes
[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10151 to look at the new patch set (#10). Change subject: [tools] ksck improvements [6/n]: Refactor result handling .. [tools] ksck improvements [6/n]: Refactor result handling This patch refactors ksck so the results of the checks are collected in a new KsckResults struct. Printing results is now a matter of iterating over the KsckResults struct and formatting the information within it. Furthermore, the KsckResults struct also serves as a programmatic access point to the results of ksck, which can be used for other things like rebalancing, auto-repair, or machine-readable printing. There's also a couple of bonus changes: - Added Ksck::Run and Ksck::RunAndPrintResults methods, which simplify running all ksck checks and printing the results, as is done in the ksck CLI tool and in ClusterVerifier. - Add the changes in https://gerrit.cloudera.org/#/c/10054/, which were about to be committed but would've needed to be redone a bit to fit with this refactor. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 --- M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_remote-test.cc A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 10 files changed, 1,194 insertions(+), 886 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/10 -- 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: newpatchset Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 10 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
