Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8208 )
Change subject: itest: allow use of verifiers with EMCs that specify non-default block managers ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/8208/4/src/kudu/integration-tests/disk_failure-itest.cc File src/kudu/integration-tests/disk_failure-itest.cc: http://gerrit.cloudera.org:8080/#/c/8208/4/src/kudu/integration-tests/disk_failure-itest.cc@115 PS4, Line 115: #if defined(__linux__) : INSTANTIATE_TEST_CASE_P(DiskFailure, DiskFailureITest, ::testing::Values("file", "log")); : #else : INSTANTIATE_TEST_CASE_P(DiskFailure, DiskFailureITest, ::testing::Values("file")); : #endif Can you encapsulate the list of available block managers in a helper method within block_manager.h or some such? I think we're already repeating it in various places; would be good to have it in just one place. http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/integration-tests/log_verifier.cc File src/kudu/integration-tests/log_verifier.cc: http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/integration-tests/log_verifier.cc@31 PS3, Line 31: #include <gflags/gflags.h> : #include <gflags/gflags_declare.h> > R2 failed IWYU because gflags_declare.h was missing. OK. Could you bug Alexey about this? It doesn't make sense that IWYU would need you to include both. http://gerrit.cloudera.org:8080/#/c/8208/4/src/kudu/integration-tests/log_verifier.cc File src/kudu/integration-tests/log_verifier.cc: http://gerrit.cloudera.org:8080/#/c/8208/4/src/kudu/integration-tests/log_verifier.cc@59 PS4, Line 59: DECLARE_string(block_manager); Don't need this (or gflags_declare.h) anymore? http://gerrit.cloudera.org:8080/#/c/8208/4/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8208/4/src/kudu/mini-cluster/external_mini_cluster.cc@134 PS4, Line 134: Status ExternalMiniCluster::HandleOptions() { Maybe we can move all of this to the ExternalMiniClusterOptions constructor? Then opts_ could continue to be const. We'd have to update the ExternalMiniClusterOptions docs to say that the defaults aren't empty strings or whatever, but the defaults would provide the desired behavior, I think. Alternatively, you could convert ExternalMiniClusterOptions to a builder pattern, and then the "if the user hasn't specified anything, fill the option with useful stuff" behavior could be deferred to the Build() method (which could return a Status, thus letting you RETURN_NOT_OK on DeduceBinRoot). This isn't a huge deal though so feel free to punt if you're not interested. -- To view, visit http://gerrit.cloudera.org:8080/8208 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa44eb33d0c025830f97a2ed7583c8186f915e94 Gerrit-Change-Number: 8208 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 06 Oct 2017 04:43:27 +0000 Gerrit-HasComments: Yes
