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: (9 comments) 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: lti typo 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, timeout)); 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 = { exe_path, "--server_address", ... }; ? (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 s/Pump/Insert/ Line 103: ASSERT_OK(WaitForServersToAgree(timeout, ts_map_, tablet_id, workload.batches_completed())); 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 http://gerrit.cloudera.org:8080/4134 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae Gerrit-PatchSet: 7 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
