Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9528 )

Change subject: KUDU-2129 make ksck less scary when copying
......................................................................


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/9528/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9528/1//COMMIT_MSG@25
PS1, Line 25:
> This is the one change I have an issue with because it makes it less clear
Yeah, I think essentially we want to convey the messages:
- The table is good, DON'T WORRY ABOUT IT
- The table is recovering, KEEP AN EYE ON IT
- The table is under-replicated/unavailable, DO SOMETHING ABOUT IT

I'll go with "not healthy" for now, hopefully its proximity with the table 
summary will be enough to connect some dots.


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

http://gerrit.cloudera.org:8080/#/c/9528/1/src/kudu/tools/ksck-test.cc@322
PS1, Line 322:
> Howabout "are not healthy"?
Sure, I wanted to avoid being so direct because this could easily be seen as 
"the table is unhealthy", but I see what you mean.


http://gerrit.cloudera.org:8080/#/c/9528/1/src/kudu/tools/ksck-test.cc@402
PS1, Line 402: RT_OK(ksck_->ChecksumData(ChecksumOptions()));
             :   ASSERT_STR_CONTAINS(err_stream_.str(),
             :                       "0/9 replicas remaining (180B from disk, 
90 rows summed)");
             :
> Are these the long lines you're complaining about? Maybe we can add a helpe
Yeah, I'm going to  just reuse the printing code we have in the Ksck class.

This ended up not being as pretty as I'd hoped because TableSummary is private, 
and I'd rather not make it public for the sake of tests. Alternatively, I could 
FRIEND_TEST everything.


http://gerrit.cloudera.org:8080/#/c/9528/1/src/kudu/tools/ksck-test.cc@420
PS1, Line 420: RT_STR_CONTAINS(err_stream_.str(), 
ExpectedKsckTableSummary("test",
             :                                                                  
 /*healthy_tables=*/ 3,
             :                                                                  
 /*recovering_tables=*/ 0,
             :                                                                  
 /*underreplicated_tables=*/ 0,
             :                                                                  
 /*consensus_mismatch_tables=*/ 0,
             :                                                                  
 /*unavailable_tables=*/ 0));
             : }
> On the other hand I think the consensus matrix literals are kind of a featu
Yeah, I think the cmatrix stuff is fine, this patch doesn't really affect it 
and I agree it's helpful to have the full output.


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

http://gerrit.cloudera.org:8080/#/c/9528/1/src/kudu/tools/ksck.h@461
PS1, Line 461: // The tablet is healthy.
> Can you add a blurb like this for all non-OK statuses? Maybe like
Done


http://gerrit.cloudera.org:8080/#/c/9528/1/src/kudu/tools/ksck.h@489
PS1, Line 489: t TotalTablets() const {
             :       return healthy_tablets + recovering_tablets + 
underreplicated_tablets +
             :           consensus_mismatch_tablets + unavailable_tablets;
             :     }
             :
             :     //
> Isn't under-replicated worse than recovering?
Ah, missed that above comment.



--
To view, visit http://gerrit.cloudera.org:8080/9528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba0c1f5b5a7083cbef99d3674dfebe1457075ce0
Gerrit-Change-Number: 9528
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 21:57:06 +0000
Gerrit-HasComments: Yes

Reply via email to