Todd Lipcon has posted comments on this change.

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

Patch Set 7:

File src/kudu/tools/

PS7, Line 57: lti

PS7, Line 67: // Easy way to create a new tablet.
this comment is no longer relevant, since we're now actually using TestWorkload 
to write some data, not just create a new tablet

Line 82:   ASSERT_OK(itest::StartElection(ts_map_[leader_uuid], tablet_id, 
hrm, why for this test do we disable elections and manually elect? is it 
necessary or could we just remove those flags above and the test would still 
work? (assuming we set num_replicas to 1)

PS7, Line 89: argv.push_back
now that we have C++11 could we just write this as:

argv = {

(and if so, update down below)

Line 95:   CHECK_EQ(1, out.empty());
ASSERT_EQ("", out); so that if it's not empty, you'll see what it was equal to

PS7, Line 97: Pump

Line 103:   ASSERT_OK(WaitForServersToAgree(timeout, ts_map_, tablet_id, 
isn't this a single-server cluster? in that case I dont think we need to do any 
waiting here.

Line 105:   LOG(INFO) << "Dump tablet data : " << std::endl << out;
can we ASSERT something here? eg assert that the number of lines is at least as 
long as what workload.rows_inserted() thinks? (or maybe even exactly? I think 
the default mode for TestWorkload is that it doesn't allow errors, so the two 
should match up perfectly).

Also could assert with a regex on at least one of the lines that it has the 
proper looking format.

Line 115:   ASSERT_OK(Subprocess::Call(argv, &out));
if you're going to add the stdout capture here, add an assertion (eg that it's 
empty, or says "Deleted tablet", or whatever it's supposed to be)

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <>
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