Dinesh Bhat has posted comments on this change.

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

Patch Set 8:


TFTR again Todd, please see updated patch and responses inline.

File src/kudu/tools/kudu-ts-cli-test.cc:

PS7, Line 57: 
> typo

PS7, Line 67: 
> this comment is no longer relevant, since we're now actually using TestWork

Line 82:   // Test for 'kudu-ts-cli dump_tablet' when there is no data in tablet
> hrm, why for this test do we disable elections and manually elect? is it ne
I actually did not know the significance of those flags until only you 
indicated now, kinda overlooked them earlier. My writes were failing without 
leader election, hence added this line. Now that I think about it, does it make 
sense to keep this DumpTabletTest separate from DeleteTabletTest ? I believe 
those flags were serving some purpose for DeleteTabletTest, perhaps to prevent 
tablet-copy kicking in from leader(guessing). However removing those flags and 
removing this election still passes the test.

PS7, Line 89: }, &out));
> now that we have C++11 could we just write this as:

Line 95:     SleepFor(MonoDelta::FromMilliseconds(10));
> ASSERT_EQ("", out); so that if it's not empty, you'll see what it was equal

PS7, Line 97: kloa
> s/Pump/Insert/

Line 103:     "dump_tablet",
> isn't this a single-server cluster? in that case I dont think we need to do
StartCluster fires 3 tservers by default, we can change it to one and just 
tombstone tablet on that :
  void StartCluster(.............
                    int num_tablet_servers = 3);

Line 105:   }, &out));
> can we ASSERT something here? eg assert that the number of lines is at leas
Good idea ! although I observed that number of rows is not exactly 5, so I was 
thinking that could it also depend on how many rows per write-batch ?  We can 
assert for at least 5 rows and also regex on format. Updated likewise.

Line 115:     GetTsCliToolPath(),
> if you're going to add the stdout capture here, add an assertion (eg that i

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to