[kudu-CR] [tools] ksck improvements [6/n]: Refactor result handling

[Will Berkeley (Code Review)]

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 (Code Review)]

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 (Code Review)]

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

[Will Berkeley (Code Review)]

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 (Code Review)]

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

[Will Berkeley (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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 (Code Review)]

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

[Will Berkeley (Code Review)]

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 (Code Review)]

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

[Will Berkeley (Code Review)]

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