Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10151 )

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


Patch Set 6:

(15 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.
> Any concerns about that in terms of backward compatibility?
I don't think we make guarantees on backwards compat for tools. Also, I don't 
think anyone besides me ever used that flag. The good info comes from the 
consensus state :)

I see the point about removing it separately but if I make it a patch prior  to 
this one I'm spending time working in the "old way" and have to rejigger the 
patch to match those changes. Seems wasteful. If I make it a patch after this 
one I have to redo pieces of this patch to re-introduce it, just so I can take 
it out.

Ergo, unless you have a big problem with keeping it part of this patch, I'd 
rather leave it in.


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/integration-tests/cluster_verifier.cc
File src/kudu/integration-tests/cluster_verifier.cc:

http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/integration-tests/cluster_verifier.cc@112
PS5, Line 112: RunAndPrintResults
> Would it make sense to separate that in two methods:
Done


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

http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@845
PS5, Line 845:                                                
/*underreplicated_tables=*/ 3,
             :                                                
/*consensus_mismatch_tables=*/ 0,
             :                                                
/*unavailable_tables=*/ 0));
             : }
> Here an in other places:
Done. Plus some changes due to how I wrote RunAndPrintResults().


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@858
PS5, Line 858:   // Now engineer a consensus conflict.
             :   const shared_ptr<KsckTabletServer>& ts = 
FindOrDie(cluster_->tablet_servers_, "ts-id-1");
             :   auto& cstate
> nit: why not just
Done


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@878
PS5, Line 878:   ASSERT_STR_CONTAINS(err_stream_.str(),
             :                       ExpectedKsckTableSummary("test",
             :
> ASSERT_OK(RunKsck()) ?
Done


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

http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@31
PS5, Line 31: "kudu/util/status.h"
> why angle brackets instead of quotes?
Oops.


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@126
PS5, Line 126: const char* const ServerHealthToString(KsckServerHealth sh);
             :
> It would be nice to mention what higher score value means, i.e. if server A
Done


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@133
PS5, Line 133: f a ser
> What if server has multiple RPC endpoints?
AFAIK we routinely don't handle this situation in our code. I don't think we 
support it overall even though some places are set up for it. What would end up 
here is what the master reports as the RPC endpoint.


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@257
PS5, Line 257: ch table and tablet.
> nit: why not constant reference?
Oops.


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

http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@88
PS5, Line 88:   CHECK(uuid_label_mapping);
> static?  It seems this function can be entered multiple times during kudu C
First point: It's in an anonymous namespace, so it's functionally equivalent to 
a static function.

Second: Done.


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@124
PS5, Line 124:   switch (cr) {
> nit: maybe, return 'const char* const' instead?
Done. Also done for function above.


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@177
PS5, Line 177:   // First, report on the masters and master tablet.
> nit: add a stop
Done


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@268
PS5, Line 268: }
> nit: two extra spaces
Done


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@269
PS5, Line 269: uid : server_uuids) {
             :     const string label(1, FindOrDie(replica_labels, uuid));
> consider adding replication factor, as it's done in https://gerrit.cloudera
Done


http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@273
PS5, Line 273:       continue;
             :     }
             :     const auto& cstate = FindOrDie(cstates, uuid);
             :     AddRowForCState(label, cstate, replica_labels, &cmatrix);
             :   }
             :   out << "The consensus matrix is:" << endl;
             :   return cmatrix.PrintTo(out);
             : }
             :
             : Status PrintServerHealthSummaries(KsckServerType type,
             :                                   const 
vector<KsckServerHealthSummary>& summaries,
             :                                   ostream& out) {
             :   out << ServerTypeToString(type) << " Summary" << endl;
             :   Dat
> maybe, separate this into a function?
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: 6
Gerrit-Owner: Will Berkeley <wdberke...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Wed, 25 Apr 2018 05:55:45 +0000
Gerrit-HasComments: Yes

Reply via email to