My responses are inline. Others please feel free to chime in. General comments:
- I think HelixDataAccessor serves as a good basis for an SPI (which I have largely ignored in the wiki pages I posted). It exposes leaf nodes as HelixProperty instances and is entirely decoupled from ZooKeeper. - Things I want to avoid: class/interface/code-size explosion and overabstraction. I'm still not sure what the right balance is. ---------------------------------------- > Date: Mon, 24 Feb 2014 22:02:29 -0800 > Subject: API Review: Helix Admin - Feedback > From: [email protected] > To: [email protected] > > Hey guys, > > Here is my first set of feedback, thought I should start this in > chunks rather than all in one go, so picking 3 items at a time. > > Connection Management > > * HelixConnection seems like it belongs to API Could you clarify what you mean by "belongs to API?" > * ZKHelixConnection is a specialization specific to Zookeeper > * We need an factory pattern which allows creating Connections based > on a provider-type. A provider type could be an enum which has > Zookeeper to begin with and we can add other types later. The factory > should be a part of SPI, one implementation of SPI is Zookeeper. Do you mean: HelixConnection connection = HelixConnectionFactory.getConnection(ConnectionType.ZOOKEEPER, zkAddress); Can you write example code for this? > * Should the zkAddress be a String? Should it be a special type like > HelixProviderURL? Having something like String requires us to document > it clearly but having a a specialization like a HelixProviderURL > allows a guided building of such URL. Not sure if its an overkill but > If we use a provider URL we should allow creating them through a > builder pattern. I would say this is overkill. Each layer of abstraction is more code for the users. Most deployments know where ZK is and it's provided to them as a string. > * We need to declare exceptions when connect fails I've ignored failure cases entirely for now. > * We need to allow for a time delay for the shutdown, something like a > future.get(timeout) > I'm not super experienced with this, so could you give an example of why this is preferred to alternative methods of preemption? > > Get an Administrator > > * HelixAdministrator seems too generic, it should be > HelixClusterAdministrator given that its an admin for that cluster? > * We should have a HelixAdministrator which can be an administrator to > all clusters. How should we retrieve such an administrator? I would rather have one administrator that manages all clusters than two administrator classes. > * Having to get a connection first and then getting admin object seems > inverted. Why not allow > HelixAdmin.create(HelixProviderURL).getConnection(). It should return > a specialization of the HelixConnection i.e. HelixAdminConnection. We > could then hang specific privileges on the HelixAdminConnection and > not have them on the HelixConnection. It's in this direction so that user can share one connection while managing different roles. For instance, a single node may be a participant and administrator for one cluster, and a spectator for another. > * What happens if an administrator is already created? Should we allow > two administrators? I think this is acceptable. I would be more hesitant if a new connection was opened for each administrator, but if they're sharing the same connection, I don't think it's a big deal. > > Resource Create > > * ResourceConfig seems like API > * Should we go all the way ResourceConfiguration instead of ResourceConfig? It means more typing, but I'm fine either way. > * ResourceConfigBuilder should have > ResourceConfig.Builder().withId("string") and the remaining should > follow like in the example Sure. We'll need to validate on build() that an id was provided. > * RebalancerConfig seems like API > * Can we figure RebalancerConfig with types? So we dont have three > versions we leverage a type to generate/assemble a builder? I don't follow, can you give an example? We want a RebalancerConfig to be an arbitrary string-serializable class. We can provide provide three special RebalancerConfigs for the three built-in rebalancing modes. These (and their builders) all derive from PartitionedRebalancerConfig. > * How do we determine if the resource is already added? That should > dictate the exception. A resource is already added if its IdealState is already added. > > I think I will pause and wait for some responses/feedback on how this > medium of feedback feels before I move forward. > > Thanks, > > Sandeep
