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 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 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 separate processes"? 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_ts_flags, extra_master_flags, and num_tablet_servers are commonly used while the rest aren't. Perhaps we can roll this back to just those three, and clients that need to configure the rest can use StartClusterWithOpts? 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 move these as well, which means changing the function to pass them by value rather than by cref. 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.h? 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 FsManagerOpts? 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? 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 copied out of ExternalDaemonOptions, which muddies the class definition and adds another step for anyone who wants to add a new option. Could you explore replacing them all with a copy of the options, stored at constructor time? 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. 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? Why do we need this additional check? -- 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: Thu, 05 Oct 2017 00:35:24 +0000 Gerrit-HasComments: Yes
