HBase
  1. HBase
  2. HBASE-3134

[replication] Add the ability to enable/disable streams

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0
    • Component/s: Replication
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      This jira was initially in the scope of HBASE-2201, but was pushed out since it has low value compared to the required effort (and when want to ship 0.90.0 rather soonish).

      We need to design a way to enable/disable replication streams in a determinate fashion.

      1. HBASE-3134.patch
        8 kB
        Teruyoshi Zenmyo
      2. HBASE-3134.patch
        11 kB
        Teruyoshi Zenmyo
      3. HBASE-3134.patch
        12 kB
        Teruyoshi Zenmyo
      4. HBASE-3134.patch
        17 kB
        Teruyoshi Zenmyo
      5. 3134.txt
        17 kB
        Ted Yu
      6. 3134-v2.txt
        17 kB
        Ted Yu
      7. 3134-v3.txt
        21 kB
        Teruyoshi Zenmyo
      8. 3134-v4.txt
        25 kB
        Teruyoshi Zenmyo

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security #7 (See https://builds.apache.org/job/HBase-0.94-security/7/)
          HBASE-3134 [replication] Add the ability to enable/disable streams (Teruyoshi Zenmyo) (Revision 1308676)

          Result = SUCCESS
          larsh :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
          • /hbase/branches/0.94/src/main/ruby/hbase/replication_admin.rb
          • /hbase/branches/0.94/src/main/ruby/shell/commands/disable_peer.rb
          • /hbase/branches/0.94/src/main/ruby/shell/commands/enable_peer.rb
          • /hbase/branches/0.94/src/main/ruby/shell/commands/list_peers.rb
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security #7 (See https://builds.apache.org/job/HBase-0.94-security/7/ ) HBASE-3134 [replication] Add the ability to enable/disable streams (Teruyoshi Zenmyo) (Revision 1308676) Result = SUCCESS larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java /hbase/branches/0.94/src/main/ruby/hbase/replication_admin.rb /hbase/branches/0.94/src/main/ruby/shell/commands/disable_peer.rb /hbase/branches/0.94/src/main/ruby/shell/commands/enable_peer.rb /hbase/branches/0.94/src/main/ruby/shell/commands/list_peers.rb /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2706 (See https://builds.apache.org/job/HBase-TRUNK/2706/)
          HBASE-3134 [replication] Add the ability to enable/disable streams (Teruyoshi Zenmyo) (Revision 1308675)

          Result = FAILURE
          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.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/ReplicationSourceInterface.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/commands/disable_peer.rb
          • /hbase/trunk/src/main/ruby/shell/commands/enable_peer.rb
          • /hbase/trunk/src/main/ruby/shell/commands/list_peers.rb
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2706 (See https://builds.apache.org/job/HBase-TRUNK/2706/ ) HBASE-3134 [replication] Add the ability to enable/disable streams (Teruyoshi Zenmyo) (Revision 1308675) Result = FAILURE larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.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/ReplicationSourceInterface.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/commands/disable_peer.rb /hbase/trunk/src/main/ruby/shell/commands/enable_peer.rb /hbase/trunk/src/main/ruby/shell/commands/list_peers.rb /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #156 (See https://builds.apache.org/job/HBase-TRUNK-security/156/)
          HBASE-3134 [replication] Add the ability to enable/disable streams (Teruyoshi Zenmyo) (Revision 1308675)

          Result = FAILURE
          larsh :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.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/ReplicationSourceInterface.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/commands/disable_peer.rb
          • /hbase/trunk/src/main/ruby/shell/commands/enable_peer.rb
          • /hbase/trunk/src/main/ruby/shell/commands/list_peers.rb
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #156 (See https://builds.apache.org/job/HBase-TRUNK-security/156/ ) HBASE-3134 [replication] Add the ability to enable/disable streams (Teruyoshi Zenmyo) (Revision 1308675) Result = FAILURE larsh : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.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/ReplicationSourceInterface.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/commands/disable_peer.rb /hbase/trunk/src/main/ruby/shell/commands/enable_peer.rb /hbase/trunk/src/main/ruby/shell/commands/list_peers.rb /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #82 (See https://builds.apache.org/job/HBase-0.94/82/)
          HBASE-3134 [replication] Add the ability to enable/disable streams (Teruyoshi Zenmyo) (Revision 1308676)

          Result = FAILURE
          larsh :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
          • /hbase/branches/0.94/src/main/ruby/hbase/replication_admin.rb
          • /hbase/branches/0.94/src/main/ruby/shell/commands/disable_peer.rb
          • /hbase/branches/0.94/src/main/ruby/shell/commands/enable_peer.rb
          • /hbase/branches/0.94/src/main/ruby/shell/commands/list_peers.rb
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java
          • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #82 (See https://builds.apache.org/job/HBase-0.94/82/ ) HBASE-3134 [replication] Add the ability to enable/disable streams (Teruyoshi Zenmyo) (Revision 1308676) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java /hbase/branches/0.94/src/main/ruby/hbase/replication_admin.rb /hbase/branches/0.94/src/main/ruby/shell/commands/disable_peer.rb /hbase/branches/0.94/src/main/ruby/shell/commands/enable_peer.rb /hbase/branches/0.94/src/main/ruby/shell/commands/list_peers.rb /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
          Hide
          Lars Hofhansl added a comment -

          Committed to 0.94 and 0.96.
          Thanks for the patch Teruyoshi.
          Thanks for the reviews.

          Show
          Lars Hofhansl added a comment - Committed to 0.94 and 0.96. Thanks for the patch Teruyoshi. Thanks for the reviews.
          Hide
          Jean-Daniel Cryans added a comment -

          +1

          Show
          Jean-Daniel Cryans added a comment - +1
          Hide
          Lars Hofhansl added a comment -

          Since I sunk 0.94.0rc0, could get it in now.
          Everybody down with that?

          Show
          Lars Hofhansl added a comment - Since I sunk 0.94.0rc0, could get it in now. Everybody down with that?
          Hide
          Jean-Daniel Cryans added a comment -

          No problem, next RC then

          Show
          Jean-Daniel Cryans added a comment - No problem, next RC then
          Hide
          Lars Hofhansl added a comment -

          Let's leave it out, unless I'm pulling in HBASE-5656 (since I'd have to trigger a new build in that case).

          Show
          Lars Hofhansl added a comment - Let's leave it out, unless I'm pulling in HBASE-5656 (since I'd have to trigger a new build in that case).
          Hide
          Jean-Daniel Cryans added a comment -

          Lars, do you want to pull this into 0.94.0 or are you too far down the process?

          Show
          Jean-Daniel Cryans added a comment - Lars, do you want to pull this into 0.94.0 or are you too far down the process?
          Hide
          Lars Hofhansl added a comment -

          Hmm... TestAtomicOperation still failing. Unrelated to this change, though.

          Show
          Lars Hofhansl added a comment - Hmm... TestAtomicOperation still failing. Unrelated to this change, though.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12520058/3134-v4.txt
          against trunk revision .

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

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

          +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 appears to introduce 1 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.regionserver.TestAtomicOperation

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1318//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1318//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1318//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12520058/3134-v4.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +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 appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestAtomicOperation Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1318//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1318//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1318//console This message is automatically generated.
          Hide
          Teruyoshi Zenmyo added a comment -

          @J-D
          Thanks for the review.
          I attached the patch with a trivial modification. (The constants in the new test are modified to the static variables of the TestReplication).

          Show
          Teruyoshi Zenmyo added a comment - @J-D Thanks for the review. I attached the patch with a trivial modification. (The constants in the new test are modified to the static variables of the TestReplication).
          Hide
          Jean-Daniel Cryans added a comment -

          +1 on the patch you put up on review board Teruyoshi, can you attach it here and grant the license so that I can commit? Thanks!

          Show
          Jean-Daniel Cryans added a comment - +1 on the patch you put up on review board Teruyoshi, can you attach it here and grant the license so that I can commit? Thanks!
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3686/
          -----------------------------------------------------------

          (Updated 2012-03-23 08:16:47.333041)

          Review request for hbase.

          Changes
          -------

          This update addresses the issue found by J-D.
          A new test which fails without the modification to the shipEdits() is added to the TestReplication.

          Summary
          -------

          1. The patch introduces a znode which represents whether a peer is enabled or not. ReplicationSource skips sending entries if the replication is disabled.
          2. The "list_peers" shows state of each peer and support for

          {enable|disable}

          Peer is added.

          This addresses bug HBASE-3134.
          https://issues.apache.org/jira/browse/HBASE-3134

          Diffs (updated)


          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java 1304172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java 1304190
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java 1304190
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 1304190
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java 1304190
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java 1304190
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/hbase/replication_admin.rb 1304172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/shell/commands/disable_peer.rb 1304172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/shell/commands/enable_peer.rb 1304172
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/shell/commands/list_peers.rb 1304172
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java 1304172
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java 1304172
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java 1304172

          Diff: https://reviews.apache.org/r/3686/diff

          Testing
          -------

          1. TestReplication passed.
          2. Checked on a cluster with some updates (puts) from shell.

          Thanks,

          Teruyoshi

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3686/ ----------------------------------------------------------- (Updated 2012-03-23 08:16:47.333041) Review request for hbase. Changes ------- This update addresses the issue found by J-D. A new test which fails without the modification to the shipEdits() is added to the TestReplication. Summary ------- 1. The patch introduces a znode which represents whether a peer is enabled or not. ReplicationSource skips sending entries if the replication is disabled. 2. The "list_peers" shows state of each peer and support for {enable|disable} Peer is added. This addresses bug HBASE-3134 . https://issues.apache.org/jira/browse/HBASE-3134 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java 1304172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java 1304190 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java 1304190 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 1304190 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java 1304190 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java 1304190 http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/hbase/replication_admin.rb 1304172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/shell/commands/disable_peer.rb 1304172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/shell/commands/enable_peer.rb 1304172 http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/shell/commands/list_peers.rb 1304172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java 1304172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java 1304172 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java 1304172 Diff: https://reviews.apache.org/r/3686/diff Testing ------- 1. TestReplication passed. 2. Checked on a cluster with some updates (puts) from shell. Thanks, Teruyoshi
          Hide
          Lars Hofhansl added a comment -

          Thanks Teruyoshi, moving it to 0.94.1 for now.

          Show
          Lars Hofhansl added a comment - Thanks Teruyoshi, moving it to 0.94.1 for now.
          Hide
          Teruyoshi Zenmyo added a comment -

          I'm sorry, the above has sent on the way by mistake.

          Thanks for feedbacks and testing, J-D.
          I'll update the patch (maybe tomorrow if I can)

          @Lars
          Please push this to later version.
          I hope that 0.94 is released faster.

          Thanks

          Show
          Teruyoshi Zenmyo added a comment - I'm sorry, the above has sent on the way by mistake. Thanks for feedbacks and testing, J-D. I'll update the patch (maybe tomorrow if I can) @Lars Please push this to later version. I hope that 0.94 is released faster. Thanks
          Hide
          Teruyoshi Zenmyo added a comment -

          Thanks for feedbacks and testing J-D.
          I'll update the patch (

          Show
          Teruyoshi Zenmyo added a comment - Thanks for feedbacks and testing J-D. I'll update the patch (
          Hide
          Jean-Daniel Cryans added a comment -

          I did some more testing, couldn't find anything else.

          Show
          Jean-Daniel Cryans added a comment - I did some more testing, couldn't find anything else.
          Hide
          Jean-Daniel Cryans added a comment -

          I found one corner case:

          1. start replication with a peer
          2. insert some data
          3. kill the peer
          4. disable replication
          5. start the peer

          The issue is that when the peer is restarted the region servers will be able to replicate the batch they are currently trying to send. Here's the log:

          2012-03-21 20:40:14,775 INFO org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Slave cluster looks down: Failed setting up proxy interface org.apache.hadoop.hbase.ipc.HRegionInterface to sv4r9s44/10.4.9.44:62023 after attempts=1
          2012-03-21 20:40:15,774 DEBUG org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Since we are unable to replicate, sleeping 1000 times 9
          2012-03-21 20:40:24,786 DEBUG org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Replicating 149
          2012-03-21 20:40:30,826 INFO org.apache.hadoop.hbase.replication.regionserver.ReplicationSourceManager: Going to report log #sv4r8s38%2C62023%2C1332361576338.1332361577015 for position 303630357 in hdfs://sv4r11s38:9100/hbase/.logs/sv4r8s38,62023,1332361576338/sv4r8s38%2C62023%2C1332361576338.1332361577015
          2012-03-21 20:40:30,830 DEBUG org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Replicated in total: 149
          2012-03-21 20:40:30,830 DEBUG org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Replication is disabled, sleeping 1000 times 1
          

          Right now shipEdits won't let you throw an Exception as it catches everything, so we could change that or we could change the loop at the end of the method to stay in there until replication is re-enabled (at which point the edits will be sent directly). I'm also opened to other suggestions.

          Another thing I saw is that both enable_peer and disable_peer in the shell still show "CURRENTLY UNSUPPORTED" in their help.

          I'm sorry it took me so long to get to this jira.

          Show
          Jean-Daniel Cryans added a comment - I found one corner case: start replication with a peer insert some data kill the peer disable replication start the peer The issue is that when the peer is restarted the region servers will be able to replicate the batch they are currently trying to send. Here's the log: 2012-03-21 20:40:14,775 INFO org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Slave cluster looks down: Failed setting up proxy interface org.apache.hadoop.hbase.ipc.HRegionInterface to sv4r9s44/10.4.9.44:62023 after attempts=1 2012-03-21 20:40:15,774 DEBUG org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Since we are unable to replicate, sleeping 1000 times 9 2012-03-21 20:40:24,786 DEBUG org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Replicating 149 2012-03-21 20:40:30,826 INFO org.apache.hadoop.hbase.replication.regionserver.ReplicationSourceManager: Going to report log #sv4r8s38%2C62023%2C1332361576338.1332361577015 for position 303630357 in hdfs://sv4r11s38:9100/hbase/.logs/sv4r8s38,62023,1332361576338/sv4r8s38%2C62023%2C1332361576338.1332361577015 2012-03-21 20:40:30,830 DEBUG org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Replicated in total: 149 2012-03-21 20:40:30,830 DEBUG org.apache.hadoop.hbase.replication.regionserver.ReplicationSource: Replication is disabled, sleeping 1000 times 1 Right now shipEdits won't let you throw an Exception as it catches everything, so we could change that or we could change the loop at the end of the method to stay in there until replication is re-enabled (at which point the edits will be sent directly). I'm also opened to other suggestions. Another thing I saw is that both enable_peer and disable_peer in the shell still show "CURRENTLY UNSUPPORTED" in their help. I'm sorry it took me so long to get to this jira.
          Hide
          Jean-Daniel Cryans added a comment -

          I'm on it today.

          Show
          Jean-Daniel Cryans added a comment - I'm on it today.
          Hide
          Lars Hofhansl added a comment -

          Is this ready for commit? Otherwise I'll push it to 0.96 (or maybe 0.94.1)

          Show
          Lars Hofhansl added a comment - Is this ready for commit? Otherwise I'll push it to 0.96 (or maybe 0.94.1)
          Hide
          Ted Yu added a comment -

          @J-D:
          Can you outline the tests you were planning ?

          Show
          Ted Yu added a comment - @J-D: Can you outline the tests you were planning ?
          Hide
          Teruyoshi Zenmyo added a comment -

          I have checked on a testing environment (coexisting cluster) with some updates (puts) from shell.

          Show
          Teruyoshi Zenmyo added a comment - I have checked on a testing environment (coexisting cluster) with some updates (puts) from shell.
          Hide
          Jean-Daniel Cryans added a comment -

          Patch looks good. Was it tested outside of unit tests?

          Show
          Jean-Daniel Cryans added a comment - Patch looks good. Was it tested outside of unit tests?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12515234/3134-v3.txt
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -136 warning messages.

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

          -1 findbugs. The patch appears to introduce 159 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.client.TestAdmin
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/992//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/992//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/992//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12515234/3134-v3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 159 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.client.TestAdmin org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/992//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/992//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/992//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Patch v3 looks good.

          Show
          Ted Yu added a comment - Patch v3 looks good.
          Hide
          Teruyoshi Zenmyo added a comment -

          The patch is updated.
          The peerClusters is still HashMap because making a concurrent version would require modifications irrelevant to this ability.

          Show
          Teruyoshi Zenmyo added a comment - The patch is updated. The peerClusters is still HashMap because making a concurrent version would require modifications irrelevant to this ability.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3686/
          -----------------------------------------------------------

          (Updated 2012-02-15 12:17:50.909795)

          Review request for hbase.

          Changes
          -------

          • PeerStateTracker is moved to ReplicationPeer
          • ReplicationSource.sourceEnabled is removed
            (Each ReplicationSource checks what's in ZK through ReplicationZookeeper)
          • Existence of peer state zk nodes is checked and the zk node would be created if it does not exist.

          However, ReplicationZookeeper is still not thread safe.

          Summary
          -------

          1. The patch introduces a znode which represents whether a peer is enabled or not. ReplicationSource skips sending entries if the replication is disabled.
          2. The "list_peers" shows state of each peer and support for

          {enable|disable}

          Peer is added.

          This addresses bug HBASE-3134.
          https://issues.apache.org/jira/browse/HBASE-3134

          Diffs (updated)


          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/hbase/replication_admin.rb 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/shell/commands/list_peers.rb 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java 1244330
          http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java 1244330

          Diff: https://reviews.apache.org/r/3686/diff

          Testing
          -------

          1. TestReplication passed.
          2. Checked on a cluster with some updates (puts) from shell.

          Thanks,

          Teruyoshi

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3686/ ----------------------------------------------------------- (Updated 2012-02-15 12:17:50.909795) Review request for hbase. Changes ------- PeerStateTracker is moved to ReplicationPeer ReplicationSource.sourceEnabled is removed (Each ReplicationSource checks what's in ZK through ReplicationZookeeper) Existence of peer state zk nodes is checked and the zk node would be created if it does not exist. However, ReplicationZookeeper is still not thread safe. Summary ------- 1. The patch introduces a znode which represents whether a peer is enabled or not. ReplicationSource skips sending entries if the replication is disabled. 2. The "list_peers" shows state of each peer and support for {enable|disable} Peer is added. This addresses bug HBASE-3134 . https://issues.apache.org/jira/browse/HBASE-3134 Diffs (updated) http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/hbase/replication_admin.rb 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/main/ruby/shell/commands/list_peers.rb 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java 1244330 http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java 1244330 Diff: https://reviews.apache.org/r/3686/diff Testing ------- 1. TestReplication passed. 2. Checked on a cluster with some updates (puts) from shell. Thanks, Teruyoshi
          Hide
          Jean-Daniel Cryans added a comment -

          I think it is better to remove the peerStateTrackers and manage peers with one HashMap(the peerClusters) by moving the PeerStateTracker to ReplicationPeer

          Yeah it'd be worth trying.

          The sourceEnabled and ReplicationPeer.peerEnabled seem to have a same role. Can the sourceEnable be removed?

          Yeah sourceEnabled was a placeholder, if you think it's better in ReplicationPeer then go for it. Just don't forget to do the check in there to verify what's in ZK.

          Show
          Jean-Daniel Cryans added a comment - I think it is better to remove the peerStateTrackers and manage peers with one HashMap(the peerClusters) by moving the PeerStateTracker to ReplicationPeer Yeah it'd be worth trying. The sourceEnabled and ReplicationPeer.peerEnabled seem to have a same role. Can the sourceEnable be removed? Yeah sourceEnabled was a placeholder, if you think it's better in ReplicationPeer then go for it. Just don't forget to do the check in there to verify what's in ZK.
          Hide
          Teruyoshi Zenmyo added a comment -

          It should be changed too, originally it wasn't used by multiple threads because there was a maximum of one peer. Just to be safe.

          I think it is better to remove the peerStateTrackers and manage peers with one HashMap(the peerClusters) by moving the PeerStateTracker to ReplicationPeer. Then the peerCluster should be fixed to be thread safe.

          It seems that ReplicationSourceManager.addPeer needs some changes too

          Is it about ReplicationSourceManager.addSource?

           public ReplicationSourceInterface addSource(String id) throws IOException {
              ReplicationSourceInterface src =
                  getReplicationSource(this.conf, this.fs, this, stopper, replicating, id);
              // TODO set it to what's in ZK
              src.setSourceEnabled(true);
              synchronized (this.hlogsById) {
          

          The sourceEnabled and ReplicationPeer.peerEnabled seem to have a same role. Can the sourceEnable be removed?

          Basically when a peer exists but peer state znode doesn't exist, we should add the peer state znode.

          I will fix this in the next patch.

          Show
          Teruyoshi Zenmyo added a comment - It should be changed too, originally it wasn't used by multiple threads because there was a maximum of one peer. Just to be safe. I think it is better to remove the peerStateTrackers and manage peers with one HashMap(the peerClusters) by moving the PeerStateTracker to ReplicationPeer. Then the peerCluster should be fixed to be thread safe. It seems that ReplicationSourceManager.addPeer needs some changes too Is it about ReplicationSourceManager.addSource? public ReplicationSourceInterface addSource( String id) throws IOException { ReplicationSourceInterface src = getReplicationSource( this .conf, this .fs, this , stopper, replicating, id); // TODO set it to what's in ZK src.setSourceEnabled( true ); synchronized ( this .hlogsById) { The sourceEnabled and ReplicationPeer.peerEnabled seem to have a same role. Can the sourceEnable be removed? Basically when a peer exists but peer state znode doesn't exist, we should add the peer state znode. I will fix this in the next patch.
          Hide
          Jean-Daniel Cryans added a comment -

          Its usage is in line with that of peerClusters. Since the key is peer id and peer should be registered first, I don't see a problem here.

          It should be changed too, originally it wasn't used by multiple threads because there was a maximum of one peer. Just to be safe.

          Show
          Jean-Daniel Cryans added a comment - Its usage is in line with that of peerClusters. Since the key is peer id and peer should be registered first, I don't see a problem here. It should be changed too, originally it wasn't used by multiple threads because there was a maximum of one peer. Just to be safe.
          Hide
          Ted Yu added a comment -

          For #1 above, is it about peerStateTrackers ?

               this.peerClusters = new HashMap<String, ReplicationPeer>();
          +    this.peerStateTrackers = new HashMap<String, PeerStateTracker>();
          

          Its usage is in line with that of peerClusters. Since the key is peer id and peer should be registered first, I don't see a problem here.

          Show
          Ted Yu added a comment - For #1 above, is it about peerStateTrackers ? this .peerClusters = new HashMap< String , ReplicationPeer>(); + this .peerStateTrackers = new HashMap< String , PeerStateTracker>(); Its usage is in line with that of peerClusters. Since the key is peer id and peer should be registered first, I don't see a problem here.
          Hide
          Ted Yu added a comment -

          For #3 above, in changePeerState() etc, we should distinguish KeeperException.NoNodeException from other KeeperException's:

          +  private void changePeerState(String id, PeerState state) throws IOException {
          ...
          +    } catch (KeeperException e) {
          +      throw new IOException("Unable to change state of the peer " + id, e);
          +    }
          

          Basically when a peer exists but peer state znode doesn't exist, we should add the peer state znode.

          Show
          Ted Yu added a comment - For #3 above, in changePeerState() etc, we should distinguish KeeperException.NoNodeException from other KeeperException's: + private void changePeerState( String id, PeerState state) throws IOException { ... + } catch (KeeperException e) { + throw new IOException( "Unable to change state of the peer " + id, e); + } Basically when a peer exists but peer state znode doesn't exist, we should add the peer state znode.
          Hide
          Jean-Daniel Cryans added a comment -

          On "3134-v2.txt":

          • HashMap won't work here, the watchers are triggered in a separate thread from the rest. Need a concurrent version.
          • It seems that ReplicationSourceManager.addPeer needs some changes too
          • Finally, it's not clear what's going to happen when you apply the patch on an existing setup where the znode doesn't exist... it seems it would fail on finding the znode.

          We're progressing.

          Show
          Jean-Daniel Cryans added a comment - On "3134-v2.txt": HashMap won't work here, the watchers are triggered in a separate thread from the rest. Need a concurrent version. It seems that ReplicationSourceManager.addPeer needs some changes too Finally, it's not clear what's going to happen when you apply the patch on an existing setup where the znode doesn't exist... it seems it would fail on finding the znode. We're progressing.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12514010/3134-v2.txt
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -136 warning messages.

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

          -1 findbugs. The patch appears to introduce 156 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.io.hfile.TestHFileBlock
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/931//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/931//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/931//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12514010/3134-v2.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 156 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.io.hfile.TestHFileBlock org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/931//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/931//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/931//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Added KeeperException as cause for IOException

          Show
          Ted Yu added a comment - Added KeeperException as cause for IOException
          Hide
          Ted Yu added a comment -

          @J-D:
          Do you want to take another look at the patch ?

          Show
          Ted Yu added a comment - @J-D: Do you want to take another look at the patch ?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12513966/3134.txt
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -136 warning messages.

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

          -1 findbugs. The patch appears to introduce 156 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.io.hfile.TestHFileBlock
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/928//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/928//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/928//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12513966/3134.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 156 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.io.hfile.TestHFileBlock org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/928//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/928//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/928//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Patch from Teruyoshi

          Show
          Ted Yu added a comment - Patch from Teruyoshi
          Hide
          Teruyoshi Zenmyo added a comment -
          Show
          Teruyoshi Zenmyo added a comment - Here is the URL. https://reviews.apache.org/r/3686/
          Hide
          Ted Yu added a comment -

          @Teruyoshi:
          Can you publish the URL for review request on review board ?

          Show
          Ted Yu added a comment - @Teruyoshi: Can you publish the URL for review request on review board ?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12513828/HBASE-3134.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -136 warning messages.

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

          -1 findbugs. The patch appears to introduce 156 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.TestRegionRebalancing
          org.apache.hadoop.hbase.io.hfile.TestHFileBlock
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/924//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/924//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/924//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12513828/HBASE-3134.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 156 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.TestRegionRebalancing org.apache.hadoop.hbase.io.hfile.TestHFileBlock org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/924//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/924//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/924//console This message is automatically generated.
          Hide
          Teruyoshi Zenmyo added a comment -

          The attached file is the updated patch which is also uploaded to the reviewboard.

          Show
          Teruyoshi Zenmyo added a comment - The attached file is the updated patch which is also uploaded to the reviewboard.
          Hide
          Teruyoshi Zenmyo added a comment -

          Thanks for the feedback. I will update the patch in a couple of days.

          Show
          Teruyoshi Zenmyo added a comment - Thanks for the feedback. I will update the patch in a couple of days.
          Hide
          Jean-Daniel Cryans added a comment -

          We can't hit ZK every time we replicate in order to see what the state is, each RS instead should have a watcher and the check should be done locally. The rest looks good, thanks a lot for working on this.

          Show
          Jean-Daniel Cryans added a comment - We can't hit ZK every time we replicate in order to see what the state is, each RS instead should have a watcher and the check should be done locally. The rest looks good, thanks a lot for working on this.
          Hide
          Lars Hofhansl added a comment -

          I think this is good to go. Let's get J-D to look at it once.

          Show
          Lars Hofhansl added a comment - I think this is good to go. Let's get J-D to look at it once.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12513253/HBASE-3134.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -136 warning messages.

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

          -1 findbugs. The patch appears to introduce 156 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/903//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/903//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/903//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12513253/HBASE-3134.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -136 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 156 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/903//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/903//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/903//console This message is automatically generated.
          Hide
          Teruyoshi Zenmyo added a comment -

          The patch is updated. A test and some comments and typos are modified.

          Show
          Teruyoshi Zenmyo added a comment - The patch is updated. A test and some comments and typos are modified.
          Hide
          Lars Hofhansl added a comment -

          Nit: In the test:

          +    for (int i = 0; i < NB_RETRIES; i++) {
          +      if (i == NB_RETRIES - 1) {
          +        break;
          +      }
          

          It is equivalent to

          for (int i = 0; i < NB_RETRIES-1; i++)
          

          And since we're just waiting for some to make sure that nothing was replicated, might as well go 'round one more time and just remove the if statement.

          Also this will always wait for 5s (not a big deal but would be nice to avoid this somehow).

          TestReplication.testStartStop() does exactly the same, though.

          +1 otherwise

          Show
          Lars Hofhansl added a comment - Nit: In the test: + for ( int i = 0; i < NB_RETRIES; i++) { + if (i == NB_RETRIES - 1) { + break ; + } It is equivalent to for ( int i = 0; i < NB_RETRIES-1; i++) And since we're just waiting for some to make sure that nothing was replicated, might as well go 'round one more time and just remove the if statement. Also this will always wait for 5s (not a big deal but would be nice to avoid this somehow). TestReplication.testStartStop() does exactly the same, though. +1 otherwise
          Hide
          Ted Yu added a comment -

          @J-D:
          Do you want to take a look ?

          Show
          Ted Yu added a comment - @J-D: Do you want to take a look ?
          Hide
          Ted Yu added a comment -

          Thanks for the explanation.
          I realized that is the case after putting in my comment.

          Show
          Ted Yu added a comment - Thanks for the explanation. I realized that is the case after putting in my comment.
          Hide
          Teruyoshi Zenmyo added a comment -

          Thank you for comments and sorry for my typos.

          We should also check that the values for these two states are different.

          Could you explain the reason why we need the check?
          Peer state zk nodes and a replication state zk node have a respectively different parent node.
          The peer state zk nodes are created under each peer's zk node and the replication state zk node is created under the root replication zk node.
          Therefore, it seemed to be no problem in case the values for these two states are same.

          Show
          Teruyoshi Zenmyo added a comment - Thank you for comments and sorry for my typos. We should also check that the values for these two states are different. Could you explain the reason why we need the check? Peer state zk nodes and a replication state zk node have a respectively different parent node. The peer state zk nodes are created under each peer's zk node and the replication state zk node is created under the root replication zk node. Therefore, it seemed to be no problem in case the values for these two states are same.
          Hide
          Ted Yu added a comment -

          In changePeerState():

          +          Bytes.toBytes(state.name()));
          +      LOG.info("peer " + id + " is disabled");
          

          Please put state.name() in the log.

          Show
          Ted Yu added a comment - In changePeerState(): + Bytes.toBytes(state.name())); + LOG.info( "peer " + id + " is disabled" ); Please put state.name() in the log.
          Hide
          Ted Yu added a comment -
          +          if (sleepForRetries("peer " + peerId + " is disalbed",
          

          Typo: disabled.

          +  // Values of znode which represents state of a peer
          

          Please change 'represents' to 'represent'.

          +  // Name of a node which indicate whether a peer is enabled or not
          +  private String peerStateNodeName;
          

          Javadoc should be changed to 'Name of zk node which stores peer state'

          +    this.peerStateNodeName = conf.get(
          +        "zookeeper.znode.replication.peers.state", "state");
               this.replicationStateNodeName =
                   conf.get("zookeeper.znode.replication.state", "state");
          

          "state" has been taken. Would "peer-state" be a better default ?
          We should also check that the values for these two states are different.

          Show
          Ted Yu added a comment - + if (sleepForRetries( "peer " + peerId + " is disalbed" , Typo: disabled. + // Values of znode which represents state of a peer Please change 'represents' to 'represent'. + // Name of a node which indicate whether a peer is enabled or not + private String peerStateNodeName; Javadoc should be changed to 'Name of zk node which stores peer state' + this .peerStateNodeName = conf.get( + "zookeeper.znode.replication.peers.state" , "state" ); this .replicationStateNodeName = conf.get( "zookeeper.znode.replication.state" , "state" ); "state" has been taken. Would "peer-state" be a better default ? We should also check that the values for these two states are different.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12512737/HBASE-3134.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -140 warning messages.

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

          -1 findbugs. The patch appears to introduce 157 new Findbugs (version 1.3.9) warnings.

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

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

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/886//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/886//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/886//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12512737/HBASE-3134.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 157 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/886//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/886//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/886//console This message is automatically generated.
          Hide
          Teruyoshi Zenmyo added a comment -

          The attached file is a patch with list_peers support.

          Show
          Teruyoshi Zenmyo added a comment - The attached file is a patch with list_peers support.
          Hide
          Teruyoshi Zenmyo added a comment -

          Yes. The paths to WAL files are collected in the queue of the ReplicationSource.
          I expect that it tolerates rather long suspension because the WAL rolling period would be relatively long.

          Show
          Teruyoshi Zenmyo added a comment - Yes. The paths to WAL files are collected in the queue of the ReplicationSource. I expect that it tolerates rather long suspension because the WAL rolling period would be relatively long.
          Hide
          Lars Hofhansl added a comment -

          If it's not too much extra work I think we can add that here.
          Should add support for

          {enable|disable}

          Peer to the shell as well.

          So what happens when you disable a peer and forgot to enable it? Will it collect logs forever?

          Show
          Lars Hofhansl added a comment - If it's not too much extra work I think we can add that here. Should add support for {enable|disable} Peer to the shell as well. So what happens when you disable a peer and forgot to enable it? Will it collect logs forever?
          Hide
          stack added a comment -

          Should I do this in this ticket or file another issue?

          Up to you.

          Show
          stack added a comment - Should I do this in this ticket or file another issue? Up to you.
          Hide
          Teruyoshi Zenmyo added a comment -

          Thanks for feedbacks.

          The difference to {add|remove}Peer is that the ReplicationSource still keeps track of the logs to be transfered to just temporarily disables shipment?

          yes

          Maybe we can show the state via ReplicationZookeeper.listPeers, which is used by the shell to show all peers.

          Should I do this in this ticket or file another issue?

          Personally I like enablePeer() and disablePeer(), because it is entirely clear what they are doing.

          I think so, too. I'd like to extract a method to remove duplicated parts.

          Show
          Teruyoshi Zenmyo added a comment - Thanks for feedbacks. The difference to {add|remove}Peer is that the ReplicationSource still keeps track of the logs to be transfered to just temporarily disables shipment? yes Maybe we can show the state via ReplicationZookeeper.listPeers, which is used by the shell to show all peers. Should I do this in this ticket or file another issue? Personally I like enablePeer() and disablePeer(), because it is entirely clear what they are doing. I think so, too. I'd like to extract a method to remove duplicated parts.
          Hide
          Lars Hofhansl added a comment -

          Just so I understand... The difference to

          {add|remove}

          Peer is that the ReplicationSource still keeps track of the logs to be transfered to just temporarily disables shipment?

          Maybe we can show the state via ReplicationZookeeper.listPeers, which is used by the shell to show all peers.

          Personally I like enablePeer() and disablePeer(), because it is entirely clear what they are doing.

          Show
          Lars Hofhansl added a comment - Just so I understand... The difference to {add|remove} Peer is that the ReplicationSource still keeps track of the logs to be transfered to just temporarily disables shipment? Maybe we can show the state via ReplicationZookeeper.listPeers, which is used by the shell to show all peers. Personally I like enablePeer() and disablePeer(), because it is entirely clear what they are doing.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12511955/HBASE-3134.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -140 warning messages.

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

          -1 findbugs. The patch appears to introduce 161 new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.client.TestFromClientSide
          org.apache.hadoop.hbase.replication.TestReplicationPeer
          org.apache.hadoop.hbase.io.hfile.TestHFileBlock
          org.apache.hadoop.hbase.mapreduce.TestImportTsv
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/855//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/855//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/855//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12511955/HBASE-3134.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -140 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 161 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.client.TestFromClientSide org.apache.hadoop.hbase.replication.TestReplicationPeer org.apache.hadoop.hbase.io.hfile.TestHFileBlock org.apache.hadoop.hbase.mapreduce.TestImportTsv org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/855//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/855//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/855//console This message is automatically generated.
          Hide
          Ted Yu added a comment -
          +      ZKUtil.deleteNode(this.zookeeper, getPeerStateZNode(id));
          

          There might be confusion because whether the peer is enabled/disabled is represented by the presence of the peer state znode. A better way is to store data in corresponding peer state znode.

          I also see similarity between enablePeer() and disablePeer(). Is it possible to create a single method, changePeerState(String id, ChangeType ct) where ChangeType is an enum indicating what to change ?

          Uploading the patch onto reviewboard would allow other people to give more precise reviews.

          Thanks

          Show
          Ted Yu added a comment - + ZKUtil.deleteNode( this .zookeeper, getPeerStateZNode(id)); There might be confusion because whether the peer is enabled/disabled is represented by the presence of the peer state znode. A better way is to store data in corresponding peer state znode. I also see similarity between enablePeer() and disablePeer(). Is it possible to create a single method, changePeerState(String id, ChangeType ct) where ChangeType is an enum indicating what to change ? Uploading the patch onto reviewboard would allow other people to give more precise reviews. Thanks
          Hide
          Teruyoshi Zenmyo added a comment -

          The patch introduces a znode which indicates that replication to a peer is disabled. ReplicationSource skips sending entries if the znode exists.

          Show
          Teruyoshi Zenmyo added a comment - The patch introduces a znode which indicates that replication to a peer is disabled. ReplicationSource skips sending entries if the znode exists.
          Hide
          stack added a comment -

          Moving out of 0.92. Move it back in if you think differently.

          Show
          stack added a comment - Moving out of 0.92. Move it back in if you think differently.
          Hide
          stack added a comment -

          Moving out of 0.92. Move it back in if you think differently.

          Show
          stack added a comment - Moving out of 0.92. Move it back in if you think differently.

            People

            • Assignee:
              Teruyoshi Zenmyo
              Reporter:
              Jean-Daniel Cryans
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development