Adar Dembo has posted comments on this change.

Change subject: Fix kudu-ts-cli crash when there is no data in tablet

Patch Set 9:


Ugh, I forgot to publish the comments I had last night. I think Todd ended up 
suggesting more or less the same thing, but I'll publish these anyway in case 
they're useful.
File src/kudu/tools/

PS7, Line 57: 
Nit: "deleting"

Line 66:   workload.Setup(); // Easy way to create a new tablet.
Add a comment explaining why the batch size is being set to 1.

Line 95: // Test dumping a tablet using kudu-ts-cli tool.
What's being tested here? If you want to assert that stdout is empty, do 

Line 105:   vector<tserver::ListTabletsResponsePB::StatusAndSchemaPB> tablets;
You're not actually testing the output, so why bother capturing it?

Actually, what is being tested with the second dump_tablet? It's not obvious, 
so please note it in a comment.

PS7, Line 108:   ASSERT_OK(i
You can reformat this like so:

  }, &out));

Then you don't need to clear argv.
File src/kudu/tools/

Line 283:     // Nothing to process from this scan result
Nit: comments that read like English sentences (i.e. begin with a capital 
letter) should be terminated with a period.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to