Dinesh Bhat has posted comments on this change.

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

Patch Set 10:


TFTR folks, updated the diffs after a round of testing again. This addresses 
all the rev comments so far on the patch. Also note that I kept the 
DumpTabletTest separate instead of squeezing into the original DeleteTabletTest 
after jenkins warned about a regression I intro'ed here :

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

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 82:     "--server_address",
> I actually did not know the significance of those flags until only you indi
I did end up adding a DumpTabletTest instead of squeezing it inside 
DeleteTabletTest because one of the jenkins test showed that tablet-copy could 
kick-in failing our expectations about TOMBSTONED state in the end.

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 AS
ASSERT_EQ also threw the erroneous string, hence prolly useful.

Line 105:   vector<tserver::ListTabletsResponsePB::StatusAndSchemaPB> tablets;
> You're not actually testing the output, so why bother capturing it?
Done, updated them.

PS7, Line 108:   ASSERT_OK(i
> You can reformat this like so:
Hmmm, yeah many of this were addressed yesterday night after Todd commented, so 
yeah already done.

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

Line 21: 
> Do we really need this header included here?
I removed it now, since I changed from ASSERT_THAT to ASSERT_STR_MATCHES which 
requires a different header now.

PS8, Line 80: ASSERT_O
> I cannot see it's used in the code below.

PS8, Line 99:   NO_FATALS(StartCluster({}, {}));
            :   TestWorkload workload(cluster_.get());
            :   workload.set_write_batch_size(1); // One batch is enough to 
dump some output.
            :   workload.Setup();
            :   vector<tserver::ListTabletsResponsePB::StatusAndSchemaPB> 
            :   for (const itest::TabletServerMap::value_type& entry : ts_map_) 
            :     TServerDetails* ts = entry.second;
            :     ASSERT_OK(itest::WaitForNumTabletsOnTS(ts, 1, timeout, 
            :   }
            :   string tablet_id = tablets[0].tablet_status().tablet_id();
> Does it make sense to separate the verification of the dump_tablet's output
I just did separate dump_tablet and delete_tablet into different tests. Also 
checking for this format with multiple ROW output now, meaning asserting for 
each row's format. As far as NULL columns go, I think TestWorkLoad generates 
non-null output, so went with whatever was coming on stdout.

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

Line 117:   string out;
> Not used?

Line 127: 
> ASSERT_TRUE(out.empty()) is more idiomatic.
Todd's point was that ASSERT_EQ also throws the unexpected string, which is 
nice to have.

Line 134:   ASSERT_OK(WaitForServersToAgree(timeout, ts_map_, tablet_id, 
> I'm not too familiar with the TestWorkload utility, but is there a race her
Looking at the code, windows seems to exist, although I don't think that's a 
problematic race though since StopAndJoin only ensures randezvous point for 
thread terminations and nothing else.

Line 145:   int nrows = 0;
> Here use the macros in test_macros.h to help out. ASSERT_STR_MATCHES() for 
yeah I saw your use of them under kudu-tool-test, but recalling some weird 
compile errors yesterday, looks like i was hallucinating :), replaced now.

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 l

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: 10
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