[ 
https://issues.apache.org/jira/browse/HBASE-10985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13980370#comment-13980370
 ] 

stack commented on HBASE-10985:
-------------------------------

Do we have to do this:

+import 
org.apache.hadoop.hbase.regionserver.consensus.SplitTransactionConsensus;

... and this:

+import 
org.apache.hadoop.hbase.regionserver.consensus.SplitTransactionConsensus;
+import 
org.apache.hadoop.hbase.regionserver.consensus.ZKSplitTransactionConsensus;

i.e. have a higher level package reach over into a package that is at a level 
below?  We'd like to not build tangles across packages w/ dependencies going 
both up and down the package hierarchy.  If under consensus we have a 
regionserver subpackage, is that a mess?  It needs to get its hands into the 
regionserver guts?

Tough problem.

Are these to be used by a subclass?

-  private Server server;
-  private ZooKeeperWatcher watcher;
+  protected Server server;
+  protected ZooKeeperWatcher watcher;
+  protected SplitTransactionConsensus splitTransactionConsensus;

Is that why access is opened up?

Is this right?  We have to pass in the watcher.  When Mikhail did his close, I 
don't think he passed in a zk instance.  Having to pass in a zk instance even 
though the consensus might not be using zk at all is implementation leaking out 
of the Interface/Implementation?

+        ConsensusProvider consensus = 
ConsensusProviderFactory.getConsensusProvider(server
+            .getConfiguration());
+        consensus.initialize(server);
+        consensus.start();
+        if 
(consensus.getSplitTransactionConsensus().transitionSplittingNode(watcher, p, 
hri_a,
+            hri_b, sn, -1, EventType.RS_ZK_REQUEST_REGION_SPLIT, 
EventType.RS_ZK_REGION_SPLITTING) == -1) {
           byte[] data = ZKAssign.getData(watcher, encodedName);

We have to initialize and then start the consensus?  (A start w/o a stop is a 
little jarring -- when I see 'start', I always expect to see a 'stop')

When I see this in SplitTransaction which is in the regionserver subpackage:

+import org.apache.hadoop.hbase.consensus.ConsensusProvider;
+import org.apache.hadoop.hbase.consensus.ConsensusProviderFactory;
+import 
org.apache.hadoop.hbase.regionserver.consensus.SplitTransactionConsensus;
+import 
org.apache.hadoop.hbase.regionserver.consensus.ZKSplitTransactionConsensus;

I wonder if we can do better?  Do we have to have this many different packages? 
 When we do they are leaking into each other.  Not good package encapsulation.

When is this called?

+  public void setConsensusProvider(ConsensusProvider consensus) {

Should it be called in the constructor for SplitTransaction?

(Is this 'consensus' we are doing here?  Or rather is it, cluster state 
transition?  we need consensus to do this?)

Be careful about tabs .... in hbase they are two spaces:

-          this.parent.getRegionNameAsString(), e);
-      }
+          consensus.getSplitTransactionConsensus().createNodeSplitting(server,
+          parent, server.getServerName(), hri_a, hri_b);

Again, the implementation is leaking here?

-      znodeVersion = getZKNode(server, services);
+      znodeVersion = 
consensus.getSplitTransactionConsensus().getZKNode(server, services, parent, 
hri_a, hri_b);

We are asking the consensus provider to do a zk op.  Would think it would be an 
op that is not named for the implementation type.

I did about 1/3rd of the patch.

Thanks.

> Decouple Split Transaction from Zookeeper
> -----------------------------------------
>
>                 Key: HBASE-10985
>                 URL: https://issues.apache.org/jira/browse/HBASE-10985
>             Project: HBase
>          Issue Type: Sub-task
>          Components: Consensus, Zookeeper
>            Reporter: Sergey Soldatov
>         Attachments: HBASE-10985.patch, HBASE-10985.patch, HBASE-10985.patch
>
>
> As part of  HBASE-10296 SplitTransaction should be decoupled from Zookeeper. 
> This is an initial patch for review. At the moment the consensus provider  
> placed directly to SplitTransaction to minimize affected code. In the ideal 
> world it should be done in HServer.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to