Adar Dembo has posted comments on this change. Change subject: Create base class for MiniCluster and ExternalMiniCluster ......................................................................
Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/3974/6/src/kudu/integration-tests/mini_cluster_base.h File src/kudu/integration-tests/mini_cluster_base.h: PS6, Line 36: Base class for MiniCluster implementations I think we should also make interfaces for Master and TabletServer (and maybe Daemon too). Just MiniClusterBase opens the door for using TEST_P() across cluster types, but interfaces for the daemons themselves would widen that further. Also doesn't have to happen now, just food for thought. Line 39: class MiniClusterBase { I don't mean to bikeshed, but I think it would be great if this were just called MiniCluster, and if the implementations were InternalMiniCluster (or InProcMiniCluster) and ExternalMiniCluster. Definitely doesn't have to happen as part of this commit, though. Line 54: I think you can add GetLeaderMasterIndex() too; it's common across both cluster types. Or, alternatively, wait until you've got an interface for Master, then add a GetLeaderMaster() method instead. -- To view, visit http://gerrit.cloudera.org:8080/3974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I62256168a9245c845e99f7253f07e69a225954bf Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
