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: (9 comments) TFTR again Todd, please see updated patch and responses inline. http://gerrit.cloudera.org:8080/#/c/4134/7/src/kudu/tools/kudu-ts-cli-test.cc File src/kudu/tools/kudu-ts-cli-test.cc: PS7, Line 57: > typo Done PS7, Line 67: > this comment is no longer relevant, since we're now actually using TestWork Done 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: Done Line 95: SleepFor(MonoDelta::FromMilliseconds(10)); > ASSERT_EQ("", out); so that if it's not empty, you'll see what it was equal Done PS7, Line 97: kloa > s/Pump/Insert/ Done 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 Done -- 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
