Alexey Serbin has posted comments on this change. Change subject: KUDU-1753 [delete_table-test] deleted-while-scanned test ......................................................................
Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/5345/6/src/kudu/integration-tests/delete_table-test.cc File src/kudu/integration-tests/delete_table-test.cc: PS6, Line 1316: shared_ptr > Nit: we could use sp::shared_ptr here to indicate that it's not C++ shared_ I'm not sure I understand what you mean. There is using kudu::client::sp::shared_ptr; in the beginning of the file. If there were an ambiguity, the compiler would give an error on this. Besides, under the hood, that _is_ shared_ptr from the STL library. In one case it's std::tr1::shared_ptr (for older compilers), in another it's just std::shared_ptr from the newer STL library. PS6, Line 1331: ON_ERROR_DO_NOT_DUMP_STACKS > Do we want to put a comment here as to why we wanted to avoid dumping stack If would better consider adding comments why it's necessary to dump stacks in other places :) The reason is simple: this is not the test which is exploring internals of the DeleteTable() mechanics. If DeleteTable() is broken for some reason, that's not the test to diagnose that sort of the breakage: there are other tests for that in this file. I'll add the comment. PS6, Line 1357: StopCluster > Nit: Wasn't AssertNoCrashes above sufficient ? Or is this added as a good p AssertNoCrashes() it not enough: it's necessary to shutdown the cluster because it's started again by the next cycle of the for() loop. Usually, StopCluster() is performed by TearDown() method of the test class, but this is not the case. If this test is re-written using parameterized test fixtures instead of the for() loop, it would not be necessary. -- To view, visit http://gerrit.cloudera.org:8080/5345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I983e68862b5535f2f95eedd41850a8a88e95e69c Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
