Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.95.0
    • Component/s: Replication
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. HBASE-7565.v1
      16 kB
      Chris Trezzo
    2. HBASE-7565.v2
      17 kB
      Chris Trezzo

      Activity

      Hide
      Chris Trezzo added a comment -

      Attached is an initial patch for the state interface. This also gives a more concrete example of what I would like to do with the peers and queues pieces.

      All the replication related tests passed locally.

      Show
      Chris Trezzo added a comment - Attached is an initial patch for the state interface. This also gives a more concrete example of what I would like to do with the peers and queues pieces. All the replication related tests passed locally.
      Hide
      Chris Trezzo added a comment -

      Submitting patch for a qa run.

      Show
      Chris Trezzo added a comment - Submitting patch for a qa run.
      Hide
      Hadoop QA added a comment -

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

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

      -1 tests included. The patch doesn't appear to include any new or modified tests.
      Please justify why no new tests are needed for this patch.
      Also please list what manual steps were performed to verify this patch.

      +1 hadoop2.0. The patch compiles against the hadoop 2.0 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 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 lineLengths. The patch introduces lines longer than 100

      -1 core tests. The patch failed these unit tests:
      org.apache.hadoop.hbase.security.token.TestZKSecretWatcher
      org.apache.hadoop.hbase.client.TestFromClientSide
      org.apache.hadoop.hbase.TestLocalHBaseCluster

      Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
      Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//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/12564831/HBASE-7565.v1 against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 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 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 lineLengths . The patch introduces lines longer than 100 -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.security.token.TestZKSecretWatcher org.apache.hadoop.hbase.client.TestFromClientSide org.apache.hadoop.hbase.TestLocalHBaseCluster Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/4015//console This message is automatically generated.
      Hide
      Sergey Shelukhin added a comment -

      stateTracker can also be final

      isStateEnabled and parse can be static

      Not clear what is the purpose of "replicating" AtomicBoolean. It's set and passed around, but only used for read in one place,
      within the same function where it's set (so the value is available without even a field being there).

      I am assuming there's no change in logic, just wrapping?

      Show
      Sergey Shelukhin added a comment - stateTracker can also be final isStateEnabled and parse can be static Not clear what is the purpose of "replicating" AtomicBoolean. It's set and passed around, but only used for read in one place, within the same function where it's set (so the value is available without even a field being there). I am assuming there's no change in logic, just wrapping?
      Hide
      Sergey Shelukhin added a comment -

      TestZKSecretWatcher appears to pass on my machine

      Show
      Sergey Shelukhin added a comment - TestZKSecretWatcher appears to pass on my machine
      Hide
      Chris Trezzo added a comment -

      Thanks Sergey Shelukhin! Correct, no change in logic, just refactoring.

      As for the AtomicBoolean, I originally thought the same and was going to remove it. The problem though is that on the Region server, it is also passed to the ReplicationSourceManager, and then in turn passed to replication sources (Check out Replication.java). I can see removing it in a future patch, and passing around ReplicationStateInterfaces instead, but I wanted to keep this first patch small. I can see doing a bunch of clean up like that once we have the three interfaces broken out.

      I'll address your other comments, investigate some of the other failures and post another patch.

      Show
      Chris Trezzo added a comment - Thanks Sergey Shelukhin ! Correct, no change in logic, just refactoring. As for the AtomicBoolean, I originally thought the same and was going to remove it. The problem though is that on the Region server, it is also passed to the ReplicationSourceManager, and then in turn passed to replication sources (Check out Replication.java). I can see removing it in a future patch, and passing around ReplicationStateInterfaces instead, but I wanted to keep this first patch small. I can see doing a bunch of clean up like that once we have the three interfaces broken out. I'll address your other comments, investigate some of the other failures and post another patch.
      Hide
      Jean-Daniel Cryans added a comment -

      I like that this function is being hidden behind an interface, but not that the internals of RZ are being exposed. Also, is it your plan to add more unit tests now that you make it easier to test?

      Show
      Jean-Daniel Cryans added a comment - I like that this function is being hidden behind an interface, but not that the internals of RZ are being exposed. Also, is it your plan to add more unit tests now that you make it easier to test?
      Hide
      Chris Trezzo added a comment -

      I think having ReplicationStateInterface inside of ReplicationZookeeper (for this first patch) is a means to an end. Eventually I could see ReplicationZookeeper going away completely. There would be three simple interfaces, and clients of replication would use the interfaces that they need. I was just trying to avoid one mega patch. Does that sound like a good plan to you, or am I missing your point Jean-Daniel Cryans?

      My plan is to also write some more unit tests, but hopefully others will join in as well .

      Chris

      Show
      Chris Trezzo added a comment - I think having ReplicationStateInterface inside of ReplicationZookeeper (for this first patch) is a means to an end. Eventually I could see ReplicationZookeeper going away completely. There would be three simple interfaces, and clients of replication would use the interfaces that they need. I was just trying to avoid one mega patch. Does that sound like a good plan to you, or am I missing your point Jean-Daniel Cryans ? My plan is to also write some more unit tests, but hopefully others will join in as well . Chris
      Hide
      Chris Trezzo added a comment -

      Attached is a second version of the patch.

      • Made stateTracker final
      • Made isStateEnabled and parse static
      • Deleted the Atomic Boolean from ReplicationZookeeper
      • Made replicationState private and exposed it through public methods.
      Show
      Chris Trezzo added a comment - Attached is a second version of the patch. Made stateTracker final Made isStateEnabled and parse static Deleted the Atomic Boolean from ReplicationZookeeper Made replicationState private and exposed it through public methods.
      Hide
      Chris Trezzo added a comment -

      I also investigated the findbugs issue (which doesn't seem to be from this patch), and checked the failed tests (which all pass locally).

      Show
      Chris Trezzo added a comment - I also investigated the findbugs issue (which doesn't seem to be from this patch), and checked the failed tests (which all pass locally).
      Hide
      Sergey Shelukhin added a comment -

      +1 on latest patch

      Show
      Sergey Shelukhin added a comment - +1 on latest patch
      Hide
      stack added a comment -

      +1 on patch. What about calling RepliationStateInterface just ReplicationState? Could do it on commit. Jean-Daniel Cryans What you think?

      Show
      stack added a comment - +1 on patch. What about calling RepliationStateInterface just ReplicationState? Could do it on commit. Jean-Daniel Cryans What you think?
      Hide
      Hadoop QA added a comment -

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

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

      -1 tests included. The patch doesn't appear to include any new or modified tests.
      Please justify why no new tests are needed for this patch.
      Also please list what manual steps were performed to verify this patch.

      +1 hadoop2.0. The patch compiles against the hadoop 2.0 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 introduces lines longer than 100

      -1 core tests. The patch failed these unit tests:
      org.apache.hadoop.hbase.client.TestMultiParallel
      org.apache.hadoop.hbase.TestLocalHBaseCluster

      Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//testReport/
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
      Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
      Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//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/12565212/HBASE-7565.v2 against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 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 introduces lines longer than 100 -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.client.TestMultiParallel org.apache.hadoop.hbase.TestLocalHBaseCluster Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/4061//console This message is automatically generated.
      Hide
      Chris Trezzo added a comment -

      Stack Jean-Daniel Cryans

      One thought that I had (relating to HBASE-7577) is to change ReplicationStateInterface to ReplicationStatus. Also change replicationStateNodeName (default: state) to "status" and change peerStateNodeName (default: peer-state) to "peer-status". My thinking behind this is that the idea of replication being ENABLED or DISABLED is referring to the status of replication, where as the state of replication should refer to all the state needed for replication (i.e. status, peers, and queues). You could then rename ReplicationZookeeper to ReplicationState, since zookeeper is becoming an implementation detail that is being pushed down another level.

      Thoughts? Or am I being fussy?

      If you guys think it is a good idea, I can post a patch with the renaming.

      Show
      Chris Trezzo added a comment - Stack Jean-Daniel Cryans One thought that I had (relating to HBASE-7577 ) is to change ReplicationStateInterface to ReplicationStatus. Also change replicationStateNodeName (default: state) to "status" and change peerStateNodeName (default: peer-state) to "peer-status". My thinking behind this is that the idea of replication being ENABLED or DISABLED is referring to the status of replication, where as the state of replication should refer to all the state needed for replication (i.e. status, peers, and queues). You could then rename ReplicationZookeeper to ReplicationState, since zookeeper is becoming an implementation detail that is being pushed down another level. Thoughts? Or am I being fussy? If you guys think it is a good idea, I can post a patch with the renaming.
      Hide
      Jean-Daniel Cryans added a comment -

      You could then rename ReplicationZookeeper to ReplicationState

      Right now RZ is also managing the queues that are up in ZK, is there another jira I missed where you extract that?

      Show
      Jean-Daniel Cryans added a comment - You could then rename ReplicationZookeeper to ReplicationState Right now RZ is also managing the queues that are up in ZK, is there another jira I missed where you extract that?
      Hide
      Chris Trezzo added a comment -

      Jean-Daniel Cryans

      Yea, check out the subtasks for HBASE-7564. There is one for peers and one for queues. I haven't finished the patches for them yet. Queues and peers is a little bit more tricky because there is a circular dependency between them.

      Show
      Chris Trezzo added a comment - Jean-Daniel Cryans Yea, check out the subtasks for HBASE-7564 . There is one for peers and one for queues. I haven't finished the patches for them yet. Queues and peers is a little bit more tricky because there is a circular dependency between them.
      Hide
      Jean-Daniel Cryans added a comment -

      Ah ok I see. So for the moment it seems the rename can wait/be done where it's more relevant. I'm +1 on the current patch.

      Show
      Jean-Daniel Cryans added a comment - Ah ok I see. So for the moment it seems the rename can wait/be done where it's more relevant. I'm +1 on the current patch.
      Hide
      Chris Trezzo added a comment -

      Sounds good to me. Thanks!

      Show
      Chris Trezzo added a comment - Sounds good to me. Thanks!
      Hide
      stack added a comment -

      Committed to trunk. Thanks for the patch Chris. Yes, we should rename dropping the 'Interface' suffix but as jd says, can be done in new issue.

      Show
      stack added a comment - Committed to trunk. Thanks for the patch Chris. Yes, we should rename dropping the 'Interface' suffix but as jd says, can be done in new issue.
      Hide
      Hudson added a comment -

      Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #359 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/359/)
      HBASE-7565 [replication] Create an interface for the replication state node (Revision 1435250)

      Result = FAILURE
      stack :
      Files :

      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java
      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationStateImpl.java
      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationStateInterface.java
      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
      Show
      Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #359 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/359/ ) HBASE-7565 [replication] Create an interface for the replication state node (Revision 1435250) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationStateImpl.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationStateInterface.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
      Hide
      Hudson added a comment -

      Integrated in HBase-TRUNK #3765 (See https://builds.apache.org/job/HBase-TRUNK/3765/)
      HBASE-7565 [replication] Create an interface for the replication state node (Revision 1435250)

      Result = FAILURE
      stack :
      Files :

      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java
      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java
      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationStateImpl.java
      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationStateInterface.java
      • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
      Show
      Hudson added a comment - Integrated in HBase-TRUNK #3765 (See https://builds.apache.org/job/HBase-TRUNK/3765/ ) HBASE-7565 [replication] Create an interface for the replication state node (Revision 1435250) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationStateImpl.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationStateInterface.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
      Hide
      stack added a comment -

      Marking closed.

      Show
      stack added a comment - Marking closed.

        People

        • Assignee:
          Chris Trezzo
          Reporter:
          Chris Trezzo
        • Votes:
          0 Vote for this issue
          Watchers:
          7 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development