HBase
  1. HBase
  2. HBASE-5904

is_enabled from shell returns differently from pre- and post- HBASE-5155

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.90.6
    • Fix Version/s: 0.90.7
    • Component/s: Zookeeper
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      This reverts a patch included in 0.90.6 that broke compatibility.

      Description

      If I launch an hbase shell that uses HBase and ZooKeeper without HBASE-5155, against HBase servers with HBASE-5155, then is_enabled for a table always returns false even if the table is considered enabled by the servers from the logs. If I then do the same thing but with an HBase shell and ZooKeeper with HBASE-5155, then is_enabled returns as expected.

      If I launch an HBase shell that uses HBase and ZooKeeper without HBASE-5155, against HBase servers also without HBASE-5155, then is_enabled works as you'd expect. But if I then do the same thing but with an HBase shell and ZooKeeper with HBASE-5155, then is_enabled returns false even though the table is considered enabled by the servers from the logs.

      Additionally, if I then try to enable the table from the HBASE-5155-containing shell, it hangs because the ZooKeeper code waits for the ZNode to be updated with "ENABLED" in the data field, but what actually happens is that the ZNode gets deleted since the servers are running without HBASE-5155.

      I think the culprit is that the indication of how a table is considered enabled inside ZooKeeper has changed with HBASE-5155. Before HBASE-5155, a table was considered enabled if the ZNode for it did not exist. After HBASE-5155, a table is considered enabled if the ZNode for it exists and has "ENABLED" in its data. I think the current code is incompatible when running clients and servers where one side has HBASE-5155 and the other side does not.

      1. HBASE-5904.patch
        27 kB
        David S. Wang

        Activity

        Hide
        stack added a comment -

        Then we should revert hbase-5155, would you agree David?

        IIRC, there was a reason for absence of znode meaning ENABLED but don't remember it off hand.

        Show
        stack added a comment - Then we should revert hbase-5155, would you agree David? IIRC, there was a reason for absence of znode meaning ENABLED but don't remember it off hand.
        Hide
        David S. Wang added a comment -

        I think at least a partial revert of HBASE-5155 is warranted here. I don't know if we want to back it out entirely as it seems to solve a race condition that would be good to not have. Perhaps most of the patch can remain, but the part that handles how a table is represented as enabled in ZK can be reverted or worked around. But Ram can comment further on how best to handle this.

        Show
        David S. Wang added a comment - I think at least a partial revert of HBASE-5155 is warranted here. I don't know if we want to back it out entirely as it seems to solve a race condition that would be good to not have. Perhaps most of the patch can remain, but the part that handles how a table is represented as enabled in ZK can be reverted or worked around. But Ram can comment further on how best to handle this.
        Hide
        David S. Wang added a comment -

        Also, I'm not sure if it matters that 0.90.6 was already cut with this change. That means that there is already an incompatible release out there. I do not know what the precedent is here or if there is one.

        Show
        David S. Wang added a comment - Also, I'm not sure if it matters that 0.90.6 was already cut with this change. That means that there is already an incompatible release out there. I do not know what the precedent is here or if there is one.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Stack
        Yes, David discussed this with me too. But i was also not sure as how to go about with this. Thanks David for bringing this up.

        Show
        ramkrishna.s.vasudevan added a comment - @Stack Yes, David discussed this with me too. But i was also not sure as how to go about with this. Thanks David for bringing this up.
        Hide
        stack added a comment -

        If 0.90.6 has this breakage, then the damage is done. We should mark hbase-5155 an incompatible change and put in a fat release note w/ how it changes behavior (Steal some of David's notes above). You up for doing this Ram?

        I'm surprised that the change in semantic where no znode no longer means enabled has not caused other issues.

        Good digging David.

        Show
        stack added a comment - If 0.90.6 has this breakage, then the damage is done. We should mark hbase-5155 an incompatible change and put in a fat release note w/ how it changes behavior (Steal some of David's notes above). You up for doing this Ram? I'm surprised that the change in semantic where no znode no longer means enabled has not caused other issues. Good digging David.
        Hide
        ramkrishna.s.vasudevan added a comment -

        Added release note to HBASE-5155.
        @David/@Stack
        Please take a look at it.

        Show
        ramkrishna.s.vasudevan added a comment - Added release note to HBASE-5155 . @David/@Stack Please take a look at it.
        Hide
        David S. Wang added a comment -

        Should we back out HBASE-5155 entirely for now? I looked at it and just backing out the part that changes the znode behavior implies that we should also remove isTablePresent(), which seems to affect more of the patch's functionality and then it gets messy.

        Is there any later change that depends on HBASE-5155?

        Show
        David S. Wang added a comment - Should we back out HBASE-5155 entirely for now? I looked at it and just backing out the part that changes the znode behavior implies that we should also remove isTablePresent(), which seems to affect more of the patch's functionality and then it gets messy. Is there any later change that depends on HBASE-5155 ?
        Hide
        stack added a comment -

        I suppose it would make sense backing it out. We could roll a 0.90.7?

        Show
        stack added a comment - I suppose it would make sense backing it out. We could roll a 0.90.7?
        Hide
        David S. Wang added a comment -

        I have a patch to back it out and will post it once I have test it more. The patch seems to make things compatible again but I want to make sure it doesn't break anything else. Look for it in a day or two.

        Show
        David S. Wang added a comment - I have a patch to back it out and will post it once I have test it more. The patch seems to make things compatible again but I want to make sure it doesn't break anything else. Look for it in a day or two.
        Hide
        stack added a comment -

        @David Ignore my query over in hbase-5155. I should probably run a 0.90.7 once it goes in...

        Show
        stack added a comment - @David Ignore my query over in hbase-5155. I should probably run a 0.90.7 once it goes in...
        Hide
        David S. Wang added a comment -

        Revert HBASE-5155. Should apply only to 0.90 branch.

        Show
        David S. Wang added a comment - Revert HBASE-5155 . Should apply only to 0.90 branch.
        Hide
        David S. Wang added a comment -

        I added a patch off of 0.90 to revert HBASE-5155. I'll throw this on RB as well, but wanted to kick off Hadoop QA bot now. Note that all unit tests for me except for org.apache.hadoop.hbase.TestZooKeeper.testClientSessionExpired, which fails fairly consistently with or without this change.

        Show
        David S. Wang added a comment - I added a patch off of 0.90 to revert HBASE-5155 . I'll throw this on RB as well, but wanted to kick off Hadoop QA bot now. Note that all unit tests for me except for org.apache.hadoop.hbase.TestZooKeeper.testClientSessionExpired, which fails fairly consistently with or without this change.
        Hide
        Hadoop QA added a comment -

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

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1791//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/12525885/HBASE-5904.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1791//console This message is automatically generated.
        Hide
        David S. Wang added a comment -

        The patch did not apply because it's against 0.90, which it seems always fails with the QA robot.

        Anyone care to take a stab at the review? I thought it was a fairly straightforward revert.

        Show
        David S. Wang added a comment - The patch did not apply because it's against 0.90, which it seems always fails with the QA robot. Anyone care to take a stab at the review? I thought it was a fairly straightforward revert.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @David
        I saw your patch at a glance. So this patch will not address HBASE-5155?

        Show
        ramkrishna.s.vasudevan added a comment - @David I saw your patch at a glance. So this patch will not address HBASE-5155 ?
        Hide
        David S. Wang added a comment -

        I thought that was what we agreed upon earlier. At least that is the impression that I got from reading the earlier comments.

        Do you think there is a way to keep the part of the patch with the actual fix, without having the incompatible ZK behavior?

        Show
        David S. Wang added a comment - I thought that was what we agreed upon earlier. At least that is the impression that I got from reading the earlier comments. Do you think there is a way to keep the part of the patch with the actual fix, without having the incompatible ZK behavior?
        Hide
        stack added a comment -

        @Ram As I read it above, we are about backing out hbase-5155 Thats what your patch does David? If so, I'm +1 on applying it to 0.90.

        Show
        stack added a comment - @Ram As I read it above, we are about backing out hbase-5155 Thats what your patch does David? If so, I'm +1 on applying it to 0.90.
        Hide
        stack added a comment -

        Making this a blocker on 0.90.7 so we don't forget it.

        Show
        stack added a comment - Making this a blocker on 0.90.7 so we don't forget it.
        Hide
        David S. Wang added a comment -

        Stack: That (backing out HBASE-5155 only) is what my patch does.

        Show
        David S. Wang added a comment - Stack: That (backing out HBASE-5155 only) is what my patch does.
        Hide
        stack added a comment -

        @Ram I will apply David's patch that backs hbase-5155 out of 0.90 tomorrow. Let us know if you have issue w/ that.

        Show
        stack added a comment - @Ram I will apply David's patch that backs hbase-5155 out of 0.90 tomorrow. Let us know if you have issue w/ that.
        Hide
        Jonathan Hsieh added a comment -

        I'm testing this patch currently (which is a revert of HBASE-5155) and plan on committing for 0.90.7 if it is clean unless I hear any complaints.

        Show
        Jonathan Hsieh added a comment - I'm testing this patch currently (which is a revert of HBASE-5155 ) and plan on committing for 0.90.7 if it is clean unless I hear any complaints.
        Hide
        Jonathan Hsieh added a comment -

        Tests pass. Will commit tomorrow.

        Show
        Jonathan Hsieh added a comment - Tests pass. Will commit tomorrow.
        Hide
        ramkrishna.s.vasudevan added a comment -

        +1.
        @Stack

        Let us know if you have issue w/ that.

        No problem.

        Show
        ramkrishna.s.vasudevan added a comment - +1. @Stack Let us know if you have issue w/ that. No problem.
        Hide
        Jonathan Hsieh added a comment -

        Thanks for the patch Dave, Thanks for reviews Stack and Ram.

        Show
        Jonathan Hsieh added a comment - Thanks for the patch Dave, Thanks for reviews Stack and Ram.

          People

          • Assignee:
            David S. Wang
            Reporter:
            David S. Wang
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development