Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.92.0
    • Component/s: Replication
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently replication supports only 1 slave cluster, need to ability to add more.

      1. 2196.txt
        10 kB
        Lars Hofhansl
      2. HBASE-2196-wip.patch
        7 kB
        Jean-Daniel Cryans
      3. 2196-v2.txt
        17 kB
        Lars Hofhansl
      4. 2196-v5.txt
        26 kB
        Lars Hofhansl
      5. 2196-v6.txt
        26 kB
        Lars Hofhansl
      6. 2196-add.txt
        2 kB
        Lars Hofhansl
      7. HBASE-2196-0.90.patch
        15 kB
        Jean-Daniel Cryans
      8. HBASE-2196-0.90-v2.patch
        26 kB
        Jean-Daniel Cryans

        Activity

        Hide
        stack stack added a comment -

        This going to be done for 0.90? If not, can we move it out?

        Show
        stack stack added a comment - This going to be done for 0.90? If not, can we move it out?
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Moving out to 0.92.0 for the moment.

        Show
        jdcryans Jean-Daniel Cryans added a comment - Moving out to 0.92.0 for the moment.
        Hide
        stack stack added a comment -

        Moving out of 0.92.0. Pull it back in if you think different.

        Show
        stack stack added a comment - Moving out of 0.92.0. Pull it back in if you think different.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        It seems the naive way would just to create multiple ReplicationSources.
        Each peer cluster would need to be handled in its own thread anyway (so that one slow peer does not slow all replication).

        From a quick pass over the code I see no reason why that would not just work.

        There would be some overhead of course that could be avoided, such as each ReplicationSource reading the same log files; these files are cached after the first read, though. As well as multiple local ZooKeeper connections, where in theory one would suffice.

        I am also probably missing something.

        Show
        lhofhansl Lars Hofhansl added a comment - It seems the naive way would just to create multiple ReplicationSources. Each peer cluster would need to be handled in its own thread anyway (so that one slow peer does not slow all replication). From a quick pass over the code I see no reason why that would not just work. There would be some overhead of course that could be avoided, such as each ReplicationSource reading the same log files; these files are cached after the first read, though. As well as multiple local ZooKeeper connections, where in theory one would suffice. I am also probably missing something.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Here's what I did. It's only a few lines of changes plus a simple test.
        It's not going to be this simple.

        I could use some input, especially regarding multi-threading and sharing zoo keeper connections.

        And there are a bunch of things I do not fully grok, yet, such as queue migration when an RS dies.

        Show
        lhofhansl Lars Hofhansl added a comment - Here's what I did. It's only a few lines of changes plus a simple test. It's not going to be this simple. I could use some input, especially regarding multi-threading and sharing zoo keeper connections. And there are a bunch of things I do not fully grok, yet, such as queue migration when an RS dies.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Hey Lars, I did some coding in March and if you're going to work on this then I should probably post my WIP. It sorta works, but it might need a rebase first.

        Show
        jdcryans Jean-Daniel Cryans added a comment - Hey Lars, I did some coding in March and if you're going to work on this then I should probably post my WIP. It sorta works, but it might need a rebase first.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Thanks J-D.
        In ReplicationSourceManager.addSource(...), why this loop:
        for (ReplicationSourceInterface source : this.sources)

        { this.zkHelper.addLogToList(name, source.getPeerClusterZnode()); }

        Instead of just adding the log of the newly added source?

        Show
        lhofhansl Lars Hofhansl added a comment - Thanks J-D. In ReplicationSourceManager.addSource(...), why this loop: for (ReplicationSourceInterface source : this.sources) { this.zkHelper.addLogToList(name, source.getPeerClusterZnode()); } Instead of just adding the log of the newly added source?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        NM... Misread the patch.

        Show
        lhofhansl Lars Hofhansl added a comment - NM... Misread the patch.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        The patch rebased and changed slightly.

        Show
        lhofhansl Lars Hofhansl added a comment - The patch rebased and changed slightly.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Might be nice also if ReplicationSource would handle their own hlogs, rather than ReplicationSourceManager managing all of them. The patch then is quite a bit larger, though.

        Show
        lhofhansl Lars Hofhansl added a comment - Might be nice also if ReplicationSource would handle their own hlogs, rather than ReplicationSourceManager managing all of them. The patch then is quite a bit larger, though.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        @J-D, are you aware of anything specific that would not work with your patch (or the "combined" patch I posted earlier)?

        Show
        lhofhansl Lars Hofhansl added a comment - @J-D, are you aware of anything specific that would not work with your patch (or the "combined" patch I posted earlier)?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        And can we get this in 0.92?

        Show
        lhofhansl Lars Hofhansl added a comment - And can we get this in 0.92?
        Hide
        stack stack added a comment -

        Pulling in. If done by friday, will commit.

        Show
        stack stack added a comment - Pulling in. If done by friday, will commit.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Might be nice also if ReplicationSource would handle their own hlogs, rather than ReplicationSourceManager managing all of them.

        Yeah somewhere along the development the design changed but not all the parts moved, feel free to try it out in the scope of a follow-up jira.

        @J-D, are you aware of anything specific that would not work with your patch (or the "combined" patch I posted earlier)?

        Have you tested it? I think it was basically done but I wanted to do more testing on real clusters before committing but it's really time-consuming. It's meant to be very simple to add multi-slave, it's just the testing part that I didn't want to be bothered with when I first wrote replication since we didn't need it back then.

        Show
        jdcryans Jean-Daniel Cryans added a comment - Might be nice also if ReplicationSource would handle their own hlogs, rather than ReplicationSourceManager managing all of them. Yeah somewhere along the development the design changed but not all the parts moved, feel free to try it out in the scope of a follow-up jira. @J-D, are you aware of anything specific that would not work with your patch (or the "combined" patch I posted earlier)? Have you tested it? I think it was basically done but I wanted to do more testing on real clusters before committing but it's really time-consuming. It's meant to be very simple to add multi-slave, it's just the testing part that I didn't want to be bothered with when I first wrote replication since we didn't need it back then.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Thanks Stack and J-D. I started on having ReplicationSource manage their own logs. So far it does not actually make the code nicer and easier to read, the version I have so far also fails TestReplication. So that's for another jira (as you say).

        One thing I did was to remove HServerAddress from ReplicationSource in favor of using ServerName.
        HServerAddress resolves hostnames right away, which is good in this case, but as HConnectionManager caches the connection anyway, that should not be a problem.

        I'll add more tests and also do real world testing, and then send an update.

        Show
        lhofhansl Lars Hofhansl added a comment - Thanks Stack and J-D. I started on having ReplicationSource manage their own logs. So far it does not actually make the code nicer and easier to read, the version I have so far also fails TestReplication. So that's for another jira (as you say). One thing I did was to remove HServerAddress from ReplicationSource in favor of using ServerName. HServerAddress resolves hostnames right away, which is good in this case, but as HConnectionManager caches the connection anyway, that should not be a problem. I'll add more tests and also do real world testing, and then send an update.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        One of my tests makes sure that no rows that existed before a peer was added is replicated to a new peer. It fails.

        But that's actually potentially the case even now, isn't it? Unless we roll the log when a peer is added, everything in latest log (which might be older) is replicated to the new peer. Correct?

        Show
        lhofhansl Lars Hofhansl added a comment - One of my tests makes sure that no rows that existed before a peer was added is replicated to a new peer. It fails. But that's actually potentially the case even now, isn't it? Unless we roll the log when a peer is added, everything in latest log (which might be older) is replicated to the new peer. Correct?
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Yes that's why I do this in TestReplication:

        for ( JVMClusterUtil.RegionServerThread r : utility1.getHBaseCluster().getRegionServerThreads()) {
          r.getRegionServer().getWAL().rollWriter();
        }
        
        Show
        jdcryans Jean-Daniel Cryans added a comment - Yes that's why I do this in TestReplication: for ( JVMClusterUtil.RegionServerThread r : utility1.getHBaseCluster().getRegionServerThreads()) { r.getRegionServer().getWAL().rollWriter(); }
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Cool. I already added a rolling to the log in my MultiSlaveReplication as well, to verify that old logs are not replicated. Thanks for the clarification.

        Show
        lhofhansl Lars Hofhansl added a comment - Cool. I already added a rolling to the log in my MultiSlaveReplication as well, to verify that old logs are not replicated. Thanks for the clarification.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        This patch work pretty well for me. The new test passes (also tests log rolling).
        And (light) testing with real clusters works too.
        I also added a list_peers command to the shell, which shows the local peer id and its cluster key.

        Show
        lhofhansl Lars Hofhansl added a comment - This patch work pretty well for me. The new test passes (also tests log rolling). And (light) testing with real clusters works too. I also added a list_peers command to the shell, which shows the local peer id and its cluster key.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        In ReplicationZookeeper.java:

        +   * @return list of all peers
        +   */
        +  public Map<String,String> listPeers() {
        

        The javadoc should match the return value of the method.

        +      ids = ZKUtil.listChildrenAndWatchThem(this.zookeeper, this.peersZNode);
        

        I think ZKUtil.listChildrenNoWatch() should suffice above.

        Also, the new convention is not to have year in file header.

        Nice work, Lars.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - In ReplicationZookeeper.java: + * @ return list of all peers + */ + public Map< String , String > listPeers() { The javadoc should match the return value of the method. + ids = ZKUtil.listChildrenAndWatchThem( this .zookeeper, this .peersZNode); I think ZKUtil.listChildrenNoWatch() should suffice above. Also, the new convention is not to have year in file header. Nice work, Lars.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        +1 after what Ted mentioned is fixed.

        Show
        jdcryans Jean-Daniel Cryans added a comment - +1 after what Ted mentioned is fixed.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Updated patch.
        The bulk of the patch is still J-D's work.

        Show
        lhofhansl Lars Hofhansl added a comment - Updated patch. The bulk of the patch is still J-D's work.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Integrated patch version 6 to TRUNK.

        Thanks for the work Lars and J-D.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Integrated patch version 6 to TRUNK. Thanks for the work Lars and J-D.
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #2213 (See https://builds.apache.org/job/HBase-TRUNK/2213/)
        HBASE-2196 Support more than one slave cluster (Lars Hofhansl)

        tedyu :
        Files :

        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
        • /hbase/trunk/src/main/ruby/hbase/replication_admin.rb
        • /hbase/trunk/src/main/ruby/shell.rb
        • /hbase/trunk/src/main/ruby/shell/commands/list_peers.rb
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestMultiSlaveReplication.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #2213 (See https://builds.apache.org/job/HBase-TRUNK/2213/ ) HBASE-2196 Support more than one slave cluster (Lars Hofhansl) tedyu : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java /hbase/trunk/src/main/ruby/hbase/replication_admin.rb /hbase/trunk/src/main/ruby/shell.rb /hbase/trunk/src/main/ruby/shell/commands/list_peers.rb /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestMultiSlaveReplication.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #2214 (See https://builds.apache.org/job/HBase-TRUNK/2214/)
        HBASE-2196 Addendum fix TestReplicationAdmin where multi-slave is supported

        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/replication/TestReplicationAdmin.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #2214 (See https://builds.apache.org/job/HBase-TRUNK/2214/ ) HBASE-2196 Addendum fix TestReplicationAdmin where multi-slave is supported tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/replication/TestReplicationAdmin.java
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Thanks Ted!

        Show
        lhofhansl Lars Hofhansl added a comment - Thanks Ted!
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Hmm... Why the addendum change to ReplicationSourceManager?
        Is there a case where stopper is null, but wasn't before?

        Should that be

        if (stopper != null && stopper.isStopped()) {

        instead?

        Show
        lhofhansl Lars Hofhansl added a comment - Hmm... Why the addendum change to ReplicationSourceManager? Is there a case where stopper is null, but wasn't before? Should that be if (stopper != null && stopper.isStopped()) { instead?
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        @Lars:
        See https://builds.apache.org/view/G-L/view/HBase/job/HBase-TRUNK/2213/testReport/junit/org.apache.hadoop.hbase.client.replication/TestReplicationAdmin/testAddRemovePeer/

        I encourage you to remove the null check and see the exact condition for the above test failure.

        Let us know your finding.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - @Lars: See https://builds.apache.org/view/G-L/view/HBase/job/HBase-TRUNK/2213/testReport/junit/org.apache.hadoop.hbase.client.replication/TestReplicationAdmin/testAddRemovePeer/ I encourage you to remove the null check and see the exact condition for the above test failure. Let us know your finding.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Thanks Ted. I think this would be a better fix, do you agree?

        And sorry for missing the obvious TestReplicationAdmin as test to run locally before attaching the patch.

        Show
        lhofhansl Lars Hofhansl added a comment - Thanks Ted. I think this would be a better fix, do you agree? And sorry for missing the obvious TestReplicationAdmin as test to run locally before attaching the patch.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        I integrated Lars' addendum to TRUNK.

        Thanks Lars.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - I integrated Lars' addendum to TRUNK. Thanks Lars.
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #2215 (See https://builds.apache.org/job/HBase-TRUNK/2215/)
        HBASE-2196 Addendum 2 from Lars which uses an anonymous Stoppable

        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/replication/TestReplicationAdmin.java
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #2215 (See https://builds.apache.org/job/HBase-TRUNK/2215/ ) HBASE-2196 Addendum 2 from Lars which uses an anonymous Stoppable tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/replication/TestReplicationAdmin.java
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Attaching a backport of this patch for 0.90

        Doing this I learned a few things:

        • The addendum as provided isn't useful. It contains a revert of a previous addendum that wasn't posted as a patch, thus if you try to apply it it fails. Not a fan.
        • The addendum that wasn't posted should have removed the "OK!" comment here:
               try {
                 admin.addPeer(ID_SECOND, KEY_SECOND);
          -      fail();
               } catch (IllegalStateException iae) {
          +      fail();
                 // OK!
               }
          

          It's ok or it's not? Had the addendum been posted, maybe someone could have catch this before.

        • I saw that the TestMultiSlaveReplication test has this copied over from another test:
          +    conf1.setStrings(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY,
          +        "org.apache.hadoop.hbase.replication.TestMasterReplication$CoprocessorCounter");
          

          I was surprised the test worked even with that, until I figured it wasn't used in this test

        Show
        jdcryans Jean-Daniel Cryans added a comment - Attaching a backport of this patch for 0.90 Doing this I learned a few things: The addendum as provided isn't useful. It contains a revert of a previous addendum that wasn't posted as a patch, thus if you try to apply it it fails. Not a fan. The addendum that wasn't posted should have removed the "OK!" comment here: try { admin.addPeer(ID_SECOND, KEY_SECOND); - fail(); } catch (IllegalStateException iae) { + fail(); // OK! } It's ok or it's not? Had the addendum been posted, maybe someone could have catch this before. I saw that the TestMultiSlaveReplication test has this copied over from another test: + conf1.setStrings(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY, + "org.apache.hadoop.hbase.replication.TestMasterReplication$CoprocessorCounter" ); I was surprised the test worked even with that, until I figured it wasn't used in this test
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Yeah, I copied stuff from my TestMasterReplication thing, and missed removing that. Sorry about that.

        Totally agree about all patches should be attached. I take full blame for that too, as it was me who didn't run a test that obviously needed to be run, and then Ted had to quick fix it for me.

        I would even suggest to make a rule and that the final patch that is committed should be attached to the jira (so that one does not have look through the jira and the review requests to find the latest patch).

        Show
        lhofhansl Lars Hofhansl added a comment - Yeah, I copied stuff from my TestMasterReplication thing, and missed removing that. Sorry about that. Totally agree about all patches should be attached. I take full blame for that too, as it was me who didn't run a test that obviously needed to be run, and then Ted had to quick fix it for me. I would even suggest to make a rule and that the final patch that is committed should be attached to the jira (so that one does not have look through the jira and the review requests to find the latest patch).
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        No worries Lars.

        I would even suggest to make a rule and that the final patch that is committed should be attached to the jira

        I'm not a big fan of rules and wouldn't force people to do it as long as was done makes sense. Else, I'll call it out, and I expect the others to do the same for me

        Show
        jdcryans Jean-Daniel Cryans added a comment - No worries Lars. I would even suggest to make a rule and that the final patch that is committed should be attached to the jira I'm not a big fan of rules and wouldn't force people to do it as long as was done makes sense. Else, I'll call it out, and I expect the others to do the same for me
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Attaching a since version of the patch, the first one was missing the 2 newly added files.

        I did a test with 3 clusters (one master, 2 slaves) and it worked flawlessly.

        Show
        jdcryans Jean-Daniel Cryans added a comment - Attaching a since version of the patch, the first one was missing the 2 newly added files. I did a test with 3 clusters (one master, 2 slaves) and it worked flawlessly.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Patch for 0.90 should have its own JIRA.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Patch for 0.90 should have its own JIRA.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        I'm not planning on committing this new feature to 0.90, I just leave those patches here so that anyone else can use them if they need to.

        Show
        jdcryans Jean-Daniel Cryans added a comment - I'm not planning on committing this new feature to 0.90, I just leave those patches here so that anyone else can use them if they need to.
        Hide
        lars_francke Lars Francke added a comment -

        This issue was closed as part of a bulk closing operation on 2015-11-20. All issues that have been resolved and where all fixVersions have been released have been closed (following discussions on the mailing list).

        Show
        lars_francke Lars Francke added a comment - This issue was closed as part of a bulk closing operation on 2015-11-20. All issues that have been resolved and where all fixVersions have been released have been closed (following discussions on the mailing list).

          People

          • Assignee:
            lhofhansl Lars Hofhansl
            Reporter:
            jdcryans Jean-Daniel Cryans
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development