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:

(6 comments)

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.

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: 
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 
ASSERT_TRUE(out.empty());


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:

  ASSERT_OK(Subprocess::Call({
    exe_path,
    "--server_address",
    cluster->tablet...,
    "delete_tablet",
    tablet_id,
    "..."
  }, &out));

Then you don't need to clear argv.


http://gerrit.cloudera.org:8080/#/c/4134/7/src/kudu/tools/ts-cli.cc
File src/kudu/tools/ts-cli.cc:

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 http://gerrit.cloudera.org:8080/4134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8470377400047236127dd409b4825c5c6ea6bbae
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@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