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

[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 printing
..


Patch Set 5:

(5 comments)

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
> Is this still relevant?  Maybe, pass it as a parameter only to the methods
We still use it internally for checksum stuff progress updates-- it's more work 
to get it out than this patch makes it look like. That's a good candidate for a 
follow-up though (I have some more planned for checksum-related stuff already).


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.h@463
PS5, Line 463: PrintResults
> As an alternative approach, what do you think about keeping the logic about
Done


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.h@478
PS5, Line 478: int t
> nit: indent
Done


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.cc@221
PS5, Line 221: Ksck::CheckMasterConsensus
> Can this method be called multiple times with the same results_ member?  If
Done


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.cc@222
PS5, Line 222: bool missing_or_conflict = false;
> nit: maybe, drop this and use results_.master_consensus_conflict instead?
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: 5
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 04:10:36 +
Gerrit-HasComments: Yes

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

[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 printing
..


Patch Set 5:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG@17
PS5, Line 17: - Removes the --consensus flag. ksck is far less useful without 
it and
:   it doesn't seem like having it brings any benefit.
> I second Alexey's concern with putting this into a different patch. I also
Yeah I'm just a curmudgeon. it was pretty easy to preserve --consensus. I 
realized I wanted to remove it and make ksck fail fast and obviously if 
consensus state gathering fails due to no perms, since too often I get ksck 
output that's useless b/c they didn't run as the kudu admin user and didn't 
look for the error messages at the top. That change is bigger and merits a 
test, so will do in a follow-up.


http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@7
PS9, Line 7: 6/n]: Refactor pr
> nit: Maybe this started out being the case, but I think this patch delves a
You're right, I wrote a placeholder initial message here before the patch 
became as much of a refactor as it did. It's more than a nit too: having a good 
commit message, and especially a good subject, is very important.


http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@9
PS9, Line 9: It separates where
   : checks are done from where results are printed
> It'd be helpful at a glance to describe what this separation is. A stab at
Done


http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@15
PS9, Line 15: running all ksck checks
> nit: could you flesh out what this means a little bit? Not having read thro
Done


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@632
PS9, Line 632: onsensus ma
> nit: spacing here and elsewhere with similar error messages above and below
This is actually intended because it's not 3 arguments- it's two, with the 
first being a long string literal broken over two lines. I couldn't find 
anything in the GSG about how many spaces to indent in this case. Anyway, I 
switched it to have each arg on a new, so the line is still annoyingly long but 
not lint-breaking.

I wish there were some tool like go's gofmt that just did the formatting and 
that's it.


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@674
PS9, Line 674: EST_F(KsckTest, Tes
> nit: spacing here and elsewhere in similar summary messages
Done


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@808
PS9, Line 808: / KUDU-2113
> nit: spacing here and in the below runtime error assertion
Do you have some kind of magic code review spectacles that let you see these 1 
space differences in tightly paked lines of code


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.h@464
PS9, Line 464:
 :   // Perform all
> nit: is there some heuristic that specifies what the proper order is?
"Do everything that fetches before you do stuff that checks, except you have to 
CheckClusterRunning to connect to the leader master before fetching. Oh, and 
you can run the master health check on its own, and the master consensus check 
after that. Also, table checks come after tablet checks."

Part of the reason for adding this method is so no other clients of Ksck need 
to remember that. It's encoded in one place- the body of the method itself.


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc@321
PS9, Line 321:  Substitute("failed to gather info for all tablet
> nit: we should probably keep the parens consistent
Done


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc@637
PS9, Line 637:
 :   int num_results = 0;
> nit: spacing
auto to the rescue.


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h
File src/kudu/tools/ksck_results.h:

http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@105
PS9, Line 105: boost::optional opid_index;
 :   boost::optional l
> Making sure I understand: this field would be non-optional, but masters, wh
Correct. And done.


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@141
PS9, Line 141:
> nit: extra .
Done


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@246
PS9, Line 246: std::vector tabl
> How do we reconcile what checks correspond to what error messages? I guess
By reading them. These are meant for 

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

[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 printing
..


Patch Set 9:

(16 comments)

Most entirely nits, I think this would have been more easily reviewable with 
some updates to the commit message.

http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG@17
PS5, Line 17: - Removes the --consensus flag. ksck is far less useful without 
it and
:   it doesn't seem like having it brings any benefit.
> I think it's not worth separating that out if the separation would bring to
I second Alexey's concern with putting this into a different patch. I also 
(less strongly) second his conclusion that it's not worth it to separate it out.


http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@7
PS9, Line 7: Refactor printing
nit: Maybe this started out being the case, but I think this patch delves a bit 
deeper into not-just-printing stuff. It's more apt to call this something like 
"refactor result handling", which I think would make it clearer to readers what 
to expect in this patch.


http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@9
PS9, Line 9: It separates where
   : checks are done from where results are printed
It'd be helpful at a glance to describe what this separation is. A stab at it:
"The various results yielded for runs of the ksck tool are now encapsulated and 
collected in the new KsckResults class. This encapsulation will..."


http://gerrit.cloudera.org:8080/#/c/10151/9//COMMIT_MSG@15
PS9, Line 15: running all ksck checks
nit: could you flesh out what this means a little bit? Not having read through 
the patch, this could mean running the CLI tool is easier, the public 
programming interface is better, etc.


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@632
PS9, Line 632:   "1 out of
nit: spacing here and elsewhere with similar error messages above and below


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@674
PS9, Line 674: ASSERT_STR_CONTAINS
nit: spacing here and elsewhere in similar summary messages


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck-test.cc@808
PS9, Line 808: ASSERT_TRUE
nit: spacing here and in the below runtime error assertion


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:

http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.h@464
PS9, Line 464:  the proper order, including checksum scans,
 :   // if enabled.
nit: is there some heuristic that specifies what the proper order is?


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc@321
PS9, Line 321: CheckClusterRunning() and FetchTableAndTabletInfo
nit: we should probably keep the parens consistent


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck.cc@637
PS9, Line 637: licaResultMap::value_type& r :
 :   FindOrDie(checksums, tablet->id()))
nit: spacing


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h
File src/kudu/tools/ksck_results.h:

http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@105
PS9, Line 105: // A config must have a term, but the master does not disclose 
it.
 :   boost::optional term;
Making sure I understand: this field would be non-optional, but masters, which 
use KsckConsensusState, do not make it public.
Is that strictly dependent on `type`? Are there checks that enforce this (e.g. 
CHECK(type == master || term) or something)?


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@141
PS9, Line 141: ..
nit: extra .


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.h@246
PS9, Line 246: std::vector error_messages;
How do we reconcile what checks correspond to what error messages? I guess the 
ordering is somewhat hard-coded, huh?


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.cc
File src/kudu/tools/ksck_results.cc:

http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/ksck_results.cc@99
PS9, Line 99:   const KsckConsensusState& cstate,
:const map 
replica_labels,
:DataTable* table) {
nit: spacing


http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:

http://gerrit.cloudera.org:8080/#/c/10151/9/src/kudu/tools/kudu-admin-test.cc@341
PS9, Line 341: 

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

[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 printing
..


Patch Set 9:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG@17
PS5, Line 17: - Removes the --consensus flag. ksck is far less useful without 
it and
:   it doesn't seem like having it brings any benefit.
> I don't think we make guarantees on backwards compat for tools. Also, I don
I think it's not worth separating that out if the separation would bring too 
much hustle and double-work.  So, I'm fine with keeping it as is.


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
Is this still relevant?  Maybe, pass it as a parameter only to the methods that 
output results of the ksck checks?


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.h@463
PS5, Line 463:
As an alternative approach, what do you think about keeping the logic about 
printing the results in KsckResults class itself?


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.h@478
PS5, Line 478:
nit: indent


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:

http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.cc@221
PS5, Line 221: lse {
Can this method be called multiple times with the same results_ member?  If 
yes, then maybe make sure results_.master_consensus_conflict is set to false in 
the beginning.


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck.cc@222
PS5, Line 222: results_.master_consensus_con
nit: maybe, drop this and use results_.master_consensus_conflict instead?



--
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: Thu, 26 Apr 2018 18:48:38 +
Gerrit-HasComments: Yes

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

[Will Berkeley (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10151

to look at the new patch set (#9).

Change subject: [tools] ksck improvements [6/n]: Refactor printing
..

[tools] ksck improvements [6/n]: Refactor printing

This patch significantly refactors printing in ksck. It separates where
checks are done from where results are printed. This separation will
make it easier to provide machine-readable output and create tools
based on the results of ksck checks, like auto-repair.

There's also a couple of bonus changes:
- Simplifies running all ksck checks and printing the results, as done
  by the ksck tool and ClusterVerifier.
- Removes the --consensus flag. ksck is far less useful without it and
  it doesn't seem like having it brings any benefit.
- 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/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 1,206 insertions(+), 892 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/9
--
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: 9
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 

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

[Will Berkeley (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/10151

to look at the new patch set (#8).

Change subject: [tools] ksck improvements [6/n]: Refactor printing
..

[tools] ksck improvements [6/n]: Refactor printing

This patch significantly refactors printing in ksck. It separates where
checks are done from where results are printed. This separation will
make it easier to provide machine-readable output and create tools
based on the results of ksck checks, like auto-repair.

There's also a couple of bonus changes:
- Simplifies running all ksck checks and printing the results, as done
  by the ksck tool and ClusterVerifier.
- Removes the --consensus flag. ksck is far less useful without it and
  it doesn't seem like having it brings any benefit.
- 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/kudu-admin-test.cc
M src/kudu/tools/tool_action_cluster.cc
M src/kudu/tools/tool_action_tablet.cc
11 files changed, 1,199 insertions(+), 890 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/8
--
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: 8
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley