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 <mpe...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to