Andrew Wong 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 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/8208/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8208/2//COMMIT_MSG@10 PS2, Line 10: using the current FLAG_block_manager of the caller process. > Nit: FLAGS_block_manager Done http://gerrit.cloudera.org:8080/#/c/8208/2//COMMIT_MSG@12 PS2, Line 12: create the cluster in a separate process > Nit: since there is more than one process, maybe "create the cluster in sep Done http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/integration-tests/external_mini_cluster-itest-base.h File src/kudu/integration-tests/external_mini_cluster-itest-base.h: http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/integration-tests/external_mini_cluster-itest-base.h@52 PS3, Line 52: void StartCluster(const std::vector<std::string>& extra_ts_flags = {}, > I think the number of params here is getting out of hand. My guess is extra Done http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/integration-tests/external_mini_cluster-itest-base.cc File src/kudu/integration-tests/external_mini_cluster-itest-base.cc: http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/integration-tests/external_mini_cluster-itest-base.cc@61 PS3, Line 61: opts.extra_master_flags = extra_master_flags; : opts.extra_tserver_flags = extra_ts_flags; > Not related to your change, but if you're interested, it would be good to m Done 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> > Why would you need both of these? Doesn't gflags.h supercede gflags_declare R2 failed IWYU because gflags_declare.h was missing. http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/integration-tests/log_verifier.cc@83 PS3, Line 83: // Change the block manager flag just long enough to open the FsManager (and, : // in turn, the block manager). > This is a little awkward. Perhaps we should add the block manager type to F Done http://gerrit.cloudera.org:8080/#/c/8208/2/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/8208/2/src/kudu/mini-cluster/external_mini_cluster.h@335 PS2, Line 335: std::string block_manager_type_; > This is in opts_; why do we need a standalone arg for it? Similar to data_root_, the default value in opts is "" to indicate `just use FLAGS_block_manager`. Although you're right this could do without duplicating in the non-default case. Done (data_root too) http://gerrit.cloudera.org:8080/#/c/8208/2/src/kudu/mini-cluster/external_mini_cluster.h@496 PS2, Line 496: const std::string block_manager_type_; > Not your fault, but we have a ton of fields here that are basically just co Rats. Realized this should have probably been in a separate patch. Can separate if that'd be helpful. Ended up piping the block manager type into the Options in from ExternalMiniClusterOptions through DataDirManagerOptions. http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/mini-cluster/external_mini_cluster.cc@31 PS3, Line 31: #include <gflags/gflags_declare.h> > Weird that you'd need this when you already include gflags.h. Ack http://gerrit.cloudera.org:8080/#/c/8208/3/src/kudu/mini-cluster/external_mini_cluster.cc@150 PS3, Line 150: #if defined(__linux__) : CHECK(block_manager_type_ == "file" || block_manager_type_ == "log"); : #else : CHECK(block_manager_type_ == "file"); : #endif > Won't the daemons fail to come up if the block manager type is unrecongized Done -- 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: 3 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 02:34:03 +0000 Gerrit-HasComments: Yes
