Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.98.1, 0.99.0
    • Component/s: Replication
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      A new configuration replication.source.per.peer.node.bandwidth is added by this jira. the default is 0 which means no throttling. the unit of this configuration is bytes-per-second.

      Description

      When we disable a peer for a time of period, and then enable it, the ReplicationSource in master cluster will push the accumulated hlog entries during the disabled interval to the re-enabled peer cluster at full speed.

      If the bandwidth of the two clusters is shared by different applications, the push at full speed for replication can use all the bandwidth and severely influence other applications.

      Though there are two config replication.source.size.capacity and replication.source.nb.capacity to tweak the batch size each time a push delivers, but if decrease these two configs, the number of pushes increase, and all these pushes proceed continuously without pause. And no obvious help for the bandwidth throttling.

      From bandwidth-sharing and push-speed perspective, it's more reasonable to provide a bandwidth up limit for each peer push channel, and within that limit, peer can choose a big batch size for each push for bandwidth efficiency.

      Any opinion?

      1. HBASE-9501-trunk_v0.patch
        4 kB
        Honghua Feng
      2. HBASE-9501-trunk_v1.patch
        11 kB
        Honghua Feng
      3. HBASE-9501-trunk_v2.patch
        10 kB
        Honghua Feng
      4. HBASE-9501-trunk_v3.patch
        12 kB
        Honghua Feng
      5. HBASE-9501-trunk_v4.patch
        12 kB
        Jean-Daniel Cryans

        Activity

        Hide
        jmspaggi Jean-Marc Spaggiari added a comment -

        I like the idea. It's like in HDFS, there is some throttle option for the under replicated blocks replication...

        Show
        jmspaggi Jean-Marc Spaggiari added a comment - I like the idea. It's like in HDFS, there is some throttle option for the under replicated blocks replication...
        Hide
        ndimiduk Nick Dimiduk added a comment -

        Having the option available to the user sounds reasonable.

        Interesting question arrises: is it the responsibility of a networked component to be a good citizen in the network, or is it up to the network to manage its citizens for QoS concerns?

        Show
        ndimiduk Nick Dimiduk added a comment - Having the option available to the user sounds reasonable. Interesting question arrises: is it the responsibility of a networked component to be a good citizen in the network, or is it up to the network to manage its citizens for QoS concerns?
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        What Nick said, also per-RS metrics like this don't take into account HBase's elastic nature... although to be fair the clusters don't tend wildly vary in size as they are running either.

        +1 if it's not too invasive, should just be a simple method call right?

        Show
        jdcryans Jean-Daniel Cryans added a comment - What Nick said, also per-RS metrics like this don't take into account HBase's elastic nature... although to be fair the clusters don't tend wildly vary in size as they are running either. +1 if it's not too invasive, should just be a simple method call right?
        Hide
        fenghh Honghua Feng added a comment -

        Nick Dimiduk / Jean-Daniel Cryans : If we need to maintain the HBase's elastic nature here, we need to maintain a current bandwidth usage in central point(master/zk) for overall bandwidth controlling. For simplicity, and since replication.source.size.capacity and replication.source.nb.capacity are also per-RS config(more strictly per-peer+per_RS), we can provide another per-RS config to throttle the up-limit of bandwidth. And I think it won't be much complex.

        If no objection, I'll provide a patch accordingly.

        Show
        fenghh Honghua Feng added a comment - Nick Dimiduk / Jean-Daniel Cryans : If we need to maintain the HBase's elastic nature here, we need to maintain a current bandwidth usage in central point(master/zk) for overall bandwidth controlling. For simplicity, and since replication.source.size.capacity and replication.source.nb.capacity are also per-RS config(more strictly per-peer+per_RS), we can provide another per-RS config to throttle the up-limit of bandwidth. And I think it won't be much complex. If no objection, I'll provide a patch accordingly.
        Hide
        fenghh Honghua Feng added a comment -

        trunk patch attached

        Show
        fenghh Honghua Feng added a comment - trunk patch attached
        Hide
        fenghh Honghua Feng added a comment -

        as this jira's description says, this feature is to prevent replication from congesting the source-peer network channel which can occur after a peer is enabled after some time disabling and it will pushes the accumulated hlog entries to peer cluster at full speed. such full-speed push without any throttling can influence other applications which share the same cluster-cluster bandwidth.

        some notes about the trunk patch:
        1. throttling cycle is 100ms, within each cycle the push size from a single source node can't exceeds (perPeerNodeBandwidth/10), unit of perPeerNodeBandwidth is bytes per second.
        2. if a single push size exceeds (perPeerNodeBandwidth/10), sleep some following cycles to amortize.
        3. by default perPeerNodeBandwidth is 0, which means by default no throttling as before, and the behavior also stays the same as before if perPeerNodeBandwidth is not explicitly configured.
        4. a unit-test testThrottling in TestReplicationSmallTests is added for verifying the throttling effect which sees >4-times delay difference when perPeerNodeBandwidth with 5-times difference configured. and by checking log added only for debugging it's ensured sleep in all possible throttling paths are covered by this unit-test.

        ping Jean-Daniel Cryans and Nick Dimiduk for review, thanks a lot

        Show
        fenghh Honghua Feng added a comment - as this jira's description says, this feature is to prevent replication from congesting the source-peer network channel which can occur after a peer is enabled after some time disabling and it will pushes the accumulated hlog entries to peer cluster at full speed. such full-speed push without any throttling can influence other applications which share the same cluster-cluster bandwidth. some notes about the trunk patch: 1. throttling cycle is 100ms, within each cycle the push size from a single source node can't exceeds (perPeerNodeBandwidth/10), unit of perPeerNodeBandwidth is bytes per second. 2. if a single push size exceeds (perPeerNodeBandwidth/10), sleep some following cycles to amortize. 3. by default perPeerNodeBandwidth is 0, which means by default no throttling as before, and the behavior also stays the same as before if perPeerNodeBandwidth is not explicitly configured. 4. a unit-test testThrottling in TestReplicationSmallTests is added for verifying the throttling effect which sees >4-times delay difference when perPeerNodeBandwidth with 5-times difference configured. and by checking log added only for debugging it's ensured sleep in all possible throttling paths are covered by this unit-test. ping Jean-Daniel Cryans and Nick Dimiduk for review, thanks a lot
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        I think the patch should be refactored so that the bandwidth limiter is just a tool you use and that tells you how long you should sleep given some information you provide. This way you don't have to start tracking how much time you've actually slept in the unit tests since this tends to be very unreliable, especially on the build machines.

        It will also make the code more readable in ReplicationSource and won't add a lot of runtime to TestReplicationSmallTests.

        Show
        jdcryans Jean-Daniel Cryans added a comment - I think the patch should be refactored so that the bandwidth limiter is just a tool you use and that tells you how long you should sleep given some information you provide. This way you don't have to start tracking how much time you've actually slept in the unit tests since this tends to be very unreliable, especially on the build machines. It will also make the code more readable in ReplicationSource and won't add a lot of runtime to TestReplicationSmallTests.
        Hide
        fenghh Honghua Feng added a comment -

        Jean-Daniel Cryans : Thanks very much for the review. New patch attached according to your review feedback. The throttling logic is refactored out to be a helper method which computes the sleep time(>0 if needed) according to the current context which are passed as parameters, the real sleep occurs in shipEdits after calling this helper function. Now the newly added unit test testApplyThrottling in TestReplicationSmallTests just checks the sleep time computed by this helper function, but without real sleep operation/call. The original added testThrottling is also run several times to verify this refactor is fine before being removed.

        Show
        fenghh Honghua Feng added a comment - Jean-Daniel Cryans : Thanks very much for the review. New patch attached according to your review feedback. The throttling logic is refactored out to be a helper method which computes the sleep time(>0 if needed) according to the current context which are passed as parameters, the real sleep occurs in shipEdits after calling this helper function. Now the newly added unit test testApplyThrottling in TestReplicationSmallTests just checks the sleep time computed by this helper function, but without real sleep operation/call. The original added testThrottling is also run several times to verify this refactor is fine before being removed.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Latest patch is looking good! Two more comments:

        • How do you feel about using a class instead of a static method? IIUC you could push the two AtomicLongs in there, move the unit test out of TestReplicationSmallTests (so that you don't have to pay the price for setUp()) and have meaningful method names instead of lines like this one:

        this.cyclePushSize.set(this.cyclePushSize.get() + currentSize);

        • The InterruptedException should be caught, if something told ReplicationSource to stop then we shouldn't ignore it and try to continue shipping edits.
        Show
        jdcryans Jean-Daniel Cryans added a comment - Latest patch is looking good! Two more comments: How do you feel about using a class instead of a static method? IIUC you could push the two AtomicLongs in there, move the unit test out of TestReplicationSmallTests (so that you don't have to pay the price for setUp()) and have meaningful method names instead of lines like this one: this.cyclePushSize.set(this.cyclePushSize.get() + currentSize); The InterruptedException should be caught, if something told ReplicationSource to stop then we shouldn't ignore it and try to continue shipping edits.
        Hide
        fenghh Honghua Feng added a comment -

        new patch per Jean-Daniel Cryans's review feedback, thanks Jean-Daniel Cryans

        Show
        fenghh Honghua Feng added a comment - new patch per Jean-Daniel Cryans 's review feedback, thanks Jean-Daniel Cryans
        Hide
        fenghh Honghua Feng added a comment -

        How do you feel about using a class instead of a static method? IIUC you could push the two AtomicLongs in there, move the unit test out of TestReplicationSmallTests (so that you don't have to pay the price for setUp()) and have meaningful method names instead of lines like this one

        ==> done

        The InterruptedException should be caught, if something told ReplicationSource to stop then we shouldn't ignore it and try to continue shipping edits

        ==> what about adding log and interrupting the current thread? same problem with ReplicationSource.sleepForRetries(). and seems there are various kinds of handling for sleep's InterruptedException in HBase, some ignores, some don't catch and delegate to upper callers...

        Show
        fenghh Honghua Feng added a comment - How do you feel about using a class instead of a static method? IIUC you could push the two AtomicLongs in there, move the unit test out of TestReplicationSmallTests (so that you don't have to pay the price for setUp()) and have meaningful method names instead of lines like this one ==> done The InterruptedException should be caught, if something told ReplicationSource to stop then we shouldn't ignore it and try to continue shipping edits ==> what about adding log and interrupting the current thread? same problem with ReplicationSource.sleepForRetries(). and seems there are various kinds of handling for sleep's InterruptedException in HBase, some ignores, some don't catch and delegate to upper callers...
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        what about adding log and interrupting the current thread?

        Sorry, that's what I meant.

        A few cosmetic comments regarding the latest patch then we can commit:

        • ReplicationThrottler.enabled and bandwidth should be final.
        • ReplicationThrottler.throttling() doesn't throttle, it just computes a sleep time. I'd name it getSleepTime().

        If you're ok with those changes I'll just apply them on commit, no need for a new patch.

        In the mean time I'm doing Submit Patch to see if everything builds correctly.

        Show
        jdcryans Jean-Daniel Cryans added a comment - what about adding log and interrupting the current thread? Sorry, that's what I meant. A few cosmetic comments regarding the latest patch then we can commit: ReplicationThrottler.enabled and bandwidth should be final. ReplicationThrottler.throttling() doesn't throttle, it just computes a sleep time. I'd name it getSleepTime(). If you're ok with those changes I'll just apply them on commit, no need for a new patch. In the mean time I'm doing Submit Patch to see if everything builds correctly.
        Hide
        fenghh Honghua Feng added a comment -

        If you're ok with those changes I'll just apply them on commit, no need for a new patch.

        Jean-Daniel Cryans: Thanks for the review, I'm ok with those changes.

        Show
        fenghh Honghua Feng added a comment - If you're ok with those changes I'll just apply them on commit, no need for a new patch. Jean-Daniel Cryans : Thanks for the review, I'm ok with those changes.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12627578/HBASE-9501-trunk_v3.patch
        against trunk revision .
        ATTACHMENT ID: 12627578

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12627578/HBASE-9501-trunk_v3.patch against trunk revision . ATTACHMENT ID: 12627578 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8637//console This message is automatically generated.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Attaching the patch I committed, just a few cosmetic changes.

        Show
        jdcryans Jean-Daniel Cryans added a comment - Attaching the patch I committed, just a few cosmetic changes.
        Hide
        jdcryans Jean-Daniel Cryans added a comment -

        Committed to 0.98.1 (or .0 if the current RC is sunk), and trunk. Thanks Honghua!

        Show
        jdcryans Jean-Daniel Cryans added a comment - Committed to 0.98.1 (or .0 if the current RC is sunk), and trunk. Thanks Honghua!
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #4906 (See https://builds.apache.org/job/HBase-TRUNK/4906/)
        HBASE-9501 Provide throttling for replication (Feng Honghua via JD) (jdcryans: rev 1566923)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationThrottler.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationThrottler.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4906 (See https://builds.apache.org/job/HBase-TRUNK/4906/ ) HBASE-9501 Provide throttling for replication (Feng Honghua via JD) (jdcryans: rev 1566923) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationThrottler.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationThrottler.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #135 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/135/)
        HBASE-9501 Provide throttling for replication (Feng Honghua via JD) (jdcryans: rev 1566922)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationThrottler.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationThrottler.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #135 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/135/ ) HBASE-9501 Provide throttling for replication (Feng Honghua via JD) (jdcryans: rev 1566922) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationThrottler.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationThrottler.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in HBase-0.98 #147 (See https://builds.apache.org/job/HBase-0.98/147/)
        HBASE-9501 Provide throttling for replication (Feng Honghua via JD) (jdcryans: rev 1566922)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationThrottler.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationThrottler.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in HBase-0.98 #147 (See https://builds.apache.org/job/HBase-0.98/147/ ) HBASE-9501 Provide throttling for replication (Feng Honghua via JD) (jdcryans: rev 1566922) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationThrottler.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationThrottler.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #87 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/87/)
        HBASE-9501 Provide throttling for replication (Feng Honghua via JD) (jdcryans: rev 1566923)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationThrottler.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationThrottler.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #87 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/87/ ) HBASE-9501 Provide throttling for replication (Feng Honghua via JD) (jdcryans: rev 1566923) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationThrottler.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationThrottler.java
        Hide
        enis Enis Soztutar added a comment -

        Closing this issue after 0.99.0 release.

        Show
        enis Enis Soztutar added a comment - Closing this issue after 0.99.0 release.

          People

          • Assignee:
            fenghh Honghua Feng
            Reporter:
            fenghh Honghua Feng
          • Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development