Andrew Wong has posted comments on this change. Change subject: minicluster: facilitate creating multidir clusters ......................................................................
Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/external_mini_cluster.cc File src/kudu/integration-tests/external_mini_cluster.cc: PS9, Line 255: > nit: not sure that's the best name for the parameter Ah! Suffix. Will use that over postfix/nums here and elsewhere. http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: PS9, Line 26: #include <vector> > This is from the system C headers, this should come the very first in this Done http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/integration-tests/multidisk-cluster-itest.cc File src/kudu/integration-tests/multidisk-cluster-itest.cc: Line 21 > nit: consider adding Done PS9, Line 26: > I don't think it's necessary. Done PS9, Line 44: > Why not just ExternalMiniClusterITestBase ? Is there anything specific y I'm planning on using this test class to test other functionality for which some of the ts_itest utilities will be useful. PS9, Line 46: > I think you can drop this empty constructor -- it will be automatically gen Done PS9, Line 93: > why not ASSERT_GT()? Done. PS9, Line 129: > I'm confused by the combination of the comment and this assert: if I'm not Agh, a very dumb mistake indeed. Operators were swapped. http://gerrit.cloudera.org:8080/#/c/6845/9/src/kudu/util/path_util.cc File src/kudu/util/path_util.cc: PS9, Line 58: vector<string> segments; : if (path[0] == '/') segments.push_back("/"); : vector<StringPiece> pieces = Split(path, "/", SkipEmpty()); : for (const StringPiece& piece : pieces) { : segments.emplace_back(piece.data(), piece.size()); : } : return segments; : } : : string DirName(const string& path) { : > It seems using JoinStrings(paths, ",") could be a better fit here. If so, Done -- To view, visit http://gerrit.cloudera.org:8080/6845 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
