Uploaded image for project: 'Phoenix'
  1. Phoenix
  2. PHOENIX-938

Use higher priority queue for index updates to prevent deadlock

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 4.0.0, 4.3.0
    • Fix Version/s: 4.1.0
    • Labels:

      Description

      With our current global secondary indexing solution, a batched Put of table data causes a RS to do a batch Put to other RSs. This has the potential to lead to a deadlock if all RS are overloaded and unable to process the pending batched Put. To prevent this, we should use a higher priority queue to submit these Puts so that they're always processed before other Puts. This will prevent the potential for a deadlock under high load. Note that this will likely require some HBase 0.98 code changes and would not be feasible to implement for HBase 0.94.

      1. phoenix-938-4.0-v0.patch
        41 kB
        Jesse Yates
      2. phoenix-938-4.0-v0.patch
        34 kB
        Jesse Yates
      3. phoenix-938-master-v0.patch
        20 kB
        Jesse Yates
      4. phoenix-938-master-v1.patch
        34 kB
        Jesse Yates
      5. phoenix-938-master-v2.patch
        59 kB
        Jesse Yates
      6. PHOENIX-938-master-v3.patch
        62 kB
        Andrew Purtell
      7. phoenix-938-master-v4.patch
        41 kB
        Jesse Yates
      8. phoenix-938-master-v5.patch
        42 kB
        Jesse Yates

        Issue Links

          Activity

          Hide
          jesse_yates Jesse Yates added a comment -

          Initial patch to support custom queue usage on both the client and server for phoenix mutable indexes. This patch has to be used in conjunction with HBASE-11048 to support client-side write priority.

          The code to support server-side is already there, you just need to specify it in the conf by:

          <property>
            <name>hbase.region.server.rpc.scheduler.factory.class</name
            <value> org.apache.hadoop.hbase.regionserver.PhoenixIndexRpcSchedulerFactory</value>
          </property>
          
          Show
          jesse_yates Jesse Yates added a comment - Initial patch to support custom queue usage on both the client and server for phoenix mutable indexes. This patch has to be used in conjunction with HBASE-11048 to support client-side write priority. The code to support server-side is already there, you just need to specify it in the conf by: <property> <name>hbase.region.server.rpc.scheduler.factory.class</name <value> org.apache.hadoop.hbase.regionserver.PhoenixIndexRpcSchedulerFactory</value> </property>
          Hide
          jamestaylor James Taylor added a comment -

          Thanks for much for the quick work on this one, Jesse Yates. You're right, though - not crazy about the synchronized block on the class. Is there an alternate way of knowing that a table is an index table? Maybe a way to register a table as an index table and a method you can call to check if a table is an index table? Maybe store a ConcurrentHashSet on the getSharedData() off of RegionCoprocessorEnvironment of the Indexer?

          Show
          jamestaylor James Taylor added a comment - Thanks for much for the quick work on this one, Jesse Yates . You're right, though - not crazy about the synchronized block on the class. Is there an alternate way of knowing that a table is an index table? Maybe a way to register a table as an index table and a method you can call to check if a table is an index table? Maybe store a ConcurrentHashSet on the getSharedData() off of RegionCoprocessorEnvironment of the Indexer?
          Hide
          jamestaylor James Taylor added a comment -

          One option that would still go through the Configuration to determine if a table is an index but require no external synchronization would be to add a configuration property with a name formed by "IDX" + tableName and a value of Boolean.TRUE. Then it's just a hash lookup to determine if a table is an index table.

          Show
          jamestaylor James Taylor added a comment - One option that would still go through the Configuration to determine if a table is an index but require no external synchronization would be to add a configuration property with a name formed by " IDX " + tableName and a value of Boolean.TRUE. Then it's just a hash lookup to determine if a table is an index table.
          Hide
          jesse_yates Jesse Yates added a comment -

          Oh, I like that much better! I knew if I just waited you would figure it out

          Show
          jesse_yates Jesse Yates added a comment - Oh, I like that much better! I knew if I just waited you would figure it out
          Hide
          jamestaylor James Taylor added a comment -

          Now don't make me code it too

          Show
          jamestaylor James Taylor added a comment - Now don't make me code it too
          Hide
          jesse_yates Jesse Yates added a comment -

          I actually wrote the code up last week, just didn't get a chance to test against latest HBase... I'll do that in the next couple days.

          Show
          jesse_yates Jesse Yates added a comment - I actually wrote the code up last week, just didn't get a chance to test against latest HBase... I'll do that in the next couple days.
          Hide
          jesse_yates Jesse Yates added a comment -

          Attaching patches for master and 4.0. Mostly just cleanup from the last iteration.

          Now that HBASE-11048 is committed to HBase trunk and 0.98 we can put this in too.

          I can put up pull requests too (for easier review) if people want.

          Show
          jesse_yates Jesse Yates added a comment - Attaching patches for master and 4.0. Mostly just cleanup from the last iteration. Now that HBASE-11048 is committed to HBase trunk and 0.98 we can put this in too. I can put up pull requests too (for easier review) if people want.
          Hide
          jesse_yates Jesse Yates added a comment -

          The other caveat is we need to update the docs to add the necessary configs to ensure the rpc scheduler on the regionserver (see first comment)

          Show
          jesse_yates Jesse Yates added a comment - The other caveat is we need to update the docs to add the necessary configs to ensure the rpc scheduler on the regionserver (see first comment)
          Hide
          jamestaylor James Taylor added a comment -

          I'm getting compiler errors with our current pom. Does this require the pom to be updated to 0.98.3, and if so, does it introduce a runtime dependency too, Jesse Yates?

          Show
          jamestaylor James Taylor added a comment - I'm getting compiler errors with our current pom. Does this require the pom to be updated to 0.98.3, and if so, does it introduce a runtime dependency too, Jesse Yates ?
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Phoenix-master-hadoop2 #3 (See https://builds.apache.org/job/Phoenix-master-hadoop2/3/)
          PHOENIX-938 Use higher priority queue for index updates to prevent deadlock (jyates) (jtaylor: rev 748b76f09ef8f16d6d028309657d8c09ea3d7822)

          • phoenix-core/src/test/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcSchedulerTest.java
          • phoenix-core/src/test/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactoryTest.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexQosRpcControllerFactory.java
          • phoenix-core/src/main/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcScheduler.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/table/CoprocessorHTableFactory.java
          • phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactory.java
          • phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexHandlerIT.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Phoenix-master-hadoop2 #3 (See https://builds.apache.org/job/Phoenix-master-hadoop2/3/ ) PHOENIX-938 Use higher priority queue for index updates to prevent deadlock (jyates) (jtaylor: rev 748b76f09ef8f16d6d028309657d8c09ea3d7822) phoenix-core/src/test/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcSchedulerTest.java phoenix-core/src/test/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactoryTest.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexQosRpcControllerFactory.java phoenix-core/src/main/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcScheduler.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/table/CoprocessorHTableFactory.java phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactory.java phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexHandlerIT.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Phoenix-master-hadoop1 #230 (See https://builds.apache.org/job/Phoenix-master-hadoop1/230/)
          PHOENIX-938 Use higher priority queue for index updates to prevent deadlock (jyates) (jtaylor: rev 748b76f09ef8f16d6d028309657d8c09ea3d7822)

          • phoenix-core/src/test/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcSchedulerTest.java
          • phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexHandlerIT.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/table/CoprocessorHTableFactory.java
          • phoenix-core/src/test/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactoryTest.java
          • phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactory.java
          • phoenix-core/src/main/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcScheduler.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexQosRpcControllerFactory.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Phoenix-master-hadoop1 #230 (See https://builds.apache.org/job/Phoenix-master-hadoop1/230/ ) PHOENIX-938 Use higher priority queue for index updates to prevent deadlock (jyates) (jtaylor: rev 748b76f09ef8f16d6d028309657d8c09ea3d7822) phoenix-core/src/test/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcSchedulerTest.java phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexHandlerIT.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/table/CoprocessorHTableFactory.java phoenix-core/src/test/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactoryTest.java phoenix-core/src/main/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactory.java phoenix-core/src/main/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcScheduler.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexQosRpcControllerFactory.java
          Hide
          jamestaylor James Taylor added a comment -

          Jesse Yates - looks like 0.98.3 is out. I'd appreciate if you could bump up the pom to this version, commit this patch, and work with mujtaba to ensure that we'll still run against 0.98.1 with this fix.

          Show
          jamestaylor James Taylor added a comment - Jesse Yates - looks like 0.98.3 is out. I'd appreciate if you could bump up the pom to this version, commit this patch, and work with mujtaba to ensure that we'll still run against 0.98.1 with this fix.
          Hide
          jesse_yates Jesse Yates added a comment -

          Started looking at this today.... at first, 0.98.3 is pretty simple. Then it starts to get complicated as 0.98.4 completely changed how rpc scheduling is implemented (HBASE-11355). I don't know if we have the bandwidth to continually monitor all the possible changes to the scheduler code to support this. Further, as we look to real transactions, this implementation becomes somewhat moot; maybe we just leave the code as-is?

          The nitty-gritty of the details is that 0.98.4 introduced the idea of an RpcExecutor (which is a great improvement over the current munging), but that isn't in 0.98.3, so we would either need to port that class to phoenix (losing any updates from the HBase community), but that's kinda already what I was doing with this patch, so maybe that's alright for now.

          Now, we could have a whole reflection framework to support the different HBase versions we are running (which becomes a testing pain, but doable) and then pick the most optimal one (0.98.4+ just uses RpcExecutor as-is, 0.98.3 uses the copied code, <=0.98.2 ignores). Or we can copy the changed implementations back and just use the same thing everywhere, but we loose out on changes... There really isn't a clean solution here :-/

          Really, this stems from the RpcScheduler code being a private interface in HBase but wanting to leverage it outside HBase.

          thoughts James Taylor?

          Show
          jesse_yates Jesse Yates added a comment - Started looking at this today.... at first, 0.98.3 is pretty simple. Then it starts to get complicated as 0.98.4 completely changed how rpc scheduling is implemented ( HBASE-11355 ). I don't know if we have the bandwidth to continually monitor all the possible changes to the scheduler code to support this. Further, as we look to real transactions, this implementation becomes somewhat moot; maybe we just leave the code as-is? The nitty-gritty of the details is that 0.98.4 introduced the idea of an RpcExecutor (which is a great improvement over the current munging), but that isn't in 0.98.3, so we would either need to port that class to phoenix (losing any updates from the HBase community), but that's kinda already what I was doing with this patch, so maybe that's alright for now. Now, we could have a whole reflection framework to support the different HBase versions we are running (which becomes a testing pain, but doable) and then pick the most optimal one (0.98.4+ just uses RpcExecutor as-is, 0.98.3 uses the copied code, <=0.98.2 ignores). Or we can copy the changed implementations back and just use the same thing everywhere, but we loose out on changes... There really isn't a clean solution here :-/ Really, this stems from the RpcScheduler code being a private interface in HBase but wanting to leverage it outside HBase. thoughts James Taylor ?
          Hide
          apurtell Andrew Purtell added a comment -

          Really, this stems from the RpcScheduler code being a private interface in HBase but wanting to leverage it outside HBase.

          What we did with HBASE-11355 is admit additional interfaces that can give some in the HBase community flexibility to experiment with RPC improvements without needing to modify core beyond that change. I think the same applies to Phoenix. You are right that these are currently private interfaces, so dislocations like you are experiencing from .3 to .4 can happen and might happen again, however by all means we could have a discussion over on dev@hbase on promoting these RPC interfaces to Public+Evolving so you have a stable leg to stand on.

          Show
          apurtell Andrew Purtell added a comment - Really, this stems from the RpcScheduler code being a private interface in HBase but wanting to leverage it outside HBase. What we did with HBASE-11355 is admit additional interfaces that can give some in the HBase community flexibility to experiment with RPC improvements without needing to modify core beyond that change. I think the same applies to Phoenix. You are right that these are currently private interfaces, so dislocations like you are experiencing from .3 to .4 can happen and might happen again, however by all means we could have a discussion over on dev@hbase on promoting these RPC interfaces to Public+Evolving so you have a stable leg to stand on.
          Hide
          apurtell Andrew Purtell added a comment -

          by all means we could have a discussion over on dev@hbase on promoting these RPC interfaces to Public+Evolving so you have a stable leg to stand on

          ... and I'm reasonably confident we'd be happy, during this promotion, to set in place additional changes if it's not quite right for your use case(s)

          Show
          apurtell Andrew Purtell added a comment - by all means we could have a discussion over on dev@hbase on promoting these RPC interfaces to Public+Evolving so you have a stable leg to stand on ... and I'm reasonably confident we'd be happy, during this promotion, to set in place additional changes if it's not quite right for your use case(s)
          Hide
          apurtell Andrew Purtell added a comment -

          I linked over to HBASE-11355 with a comment pointing here so watchers on that issue can find their way over.

          Show
          apurtell Andrew Purtell added a comment - I linked over to HBASE-11355 with a comment pointing here so watchers on that issue can find their way over.
          Hide
          jesse_yates Jesse Yates added a comment -

          Thanks Andy! Perhaps just a bit tired today to find my way to the community solution

          Show
          jesse_yates Jesse Yates added a comment - Thanks Andy! Perhaps just a bit tired today to find my way to the community solution
          Hide
          apurtell Andrew Purtell added a comment -

          Perhaps just a bit tired today to find my way to the community solution

          What can we do to help?

          Show
          apurtell Andrew Purtell added a comment - Perhaps just a bit tired today to find my way to the community solution What can we do to help?
          Hide
          jamestaylor James Taylor added a comment -

          How about this as a plan, Jesse Yates and Andrew Purtell?

          • check-in your patch and document that it fixes the issue for 0.98.3 only (assuming it doesn't break Phoenix for 0.98.2- and 0.98.3+).
          • work with HBase community to make these APIs public and evolving for 0.98.<next release>.
          • implement solution on top of these public APIs.
            I don't think transactions is really going to help with this, so it'd be good to get an as-permanent-as-possible solution IMO.
          Show
          jamestaylor James Taylor added a comment - How about this as a plan, Jesse Yates and Andrew Purtell ? check-in your patch and document that it fixes the issue for 0.98.3 only (assuming it doesn't break Phoenix for 0.98.2- and 0.98.3+). work with HBase community to make these APIs public and evolving for 0.98.<next release>. implement solution on top of these public APIs. I don't think transactions is really going to help with this, so it'd be good to get an as-permanent-as-possible solution IMO.
          Hide
          jesse_yates Jesse Yates added a comment -

          I think I can do a version that supports 0.98.3./4 - and is backwards compatible - and then I'll see if we can get those interfaces visible for future releases. Its a bit a reflection trickery, but should be alright.

          I don't think transactions is really going to help with this

          They would in that (theoretically) they won't need this mechanism, and we can do indexing ala transactions, so all this can just go away. But, until then... this will have to do.

          Show
          jesse_yates Jesse Yates added a comment - I think I can do a version that supports 0.98.3./4 - and is backwards compatible - and then I'll see if we can get those interfaces visible for future releases. Its a bit a reflection trickery, but should be alright. I don't think transactions is really going to help with this They would in that (theoretically) they won't need this mechanism, and we can do indexing ala transactions, so all this can just go away. But, until then... this will have to do.
          Hide
          jesse_yates Jesse Yates added a comment -

          And just filed HBASE-11497 to expose the interfaces we need.

          Show
          jesse_yates Jesse Yates added a comment - And just filed HBASE-11497 to expose the interfaces we need.
          Hide
          apurtell Andrew Purtell added a comment -

          Either a Public or LimitedPrivate InterfaceAudience listing "Phoenix" would be respected by the 0.98 RM. Hope that works.

          Show
          apurtell Andrew Purtell added a comment - Either a Public or LimitedPrivate InterfaceAudience listing "Phoenix" would be respected by the 0.98 RM. Hope that works.
          Hide
          jesse_yates Jesse Yates added a comment - - edited

          Well, thank you Mr. RM

          Show
          jesse_yates Jesse Yates added a comment - - edited Well, thank you Mr. RM
          Hide
          jesse_yates Jesse Yates added a comment -

          Patch that supports HBase 0.98.4 and below. Unfortunately, requires release of 0.98.4 to use the RpcExecutor that comes in 0.98.4 (to compile). We could scale it back to not use that, but then we'd just want to add it again when 0.98.4 is released... so maybe just wait?

          Also, tested this with an HBase cluster of 0.98.4,0.98.3, and 0.98.2... so it should be ok for backwards compatibility.

          Show
          jesse_yates Jesse Yates added a comment - Patch that supports HBase 0.98.4 and below. Unfortunately, requires release of 0.98.4 to use the RpcExecutor that comes in 0.98.4 (to compile). We could scale it back to not use that, but then we'd just want to add it again when 0.98.4 is released... so maybe just wait? Also, tested this with an HBase cluster of 0.98.4,0.98.3, and 0.98.2... so it should be ok for backwards compatibility.
          Hide
          apurtell Andrew Purtell added a comment -

          Unfortunately, requires release of 0.98.4 to use the RpcExecutor that comes in 0.98.4 (to compile)

          There is only HBASE-11118 as blocker for the 0.98.4 release, I'm going to try to resolve it in time to roll a release candidate next Monday, 7/14.

          Show
          apurtell Andrew Purtell added a comment - Unfortunately, requires release of 0.98.4 to use the RpcExecutor that comes in 0.98.4 (to compile) There is only HBASE-11118 as blocker for the 0.98.4 release, I'm going to try to resolve it in time to roll a release candidate next Monday, 7/14.
          Hide
          jamestaylor James Taylor added a comment -

          +1, Jesse Yates. Thanks for reworking this. The only thing I'm worried about is timing. We'll be dependent on the release of 0.98.4, so if it slips, we're screwed, as we have to release by the end of this month.

          Show
          jamestaylor James Taylor added a comment - +1, Jesse Yates . Thanks for reworking this. The only thing I'm worried about is timing. We'll be dependent on the release of 0.98.4, so if it slips, we're screwed, as we have to release by the end of this month.
          Hide
          jamestaylor James Taylor added a comment - - edited

          Jesse Yates - wrt transactions, the plumbing for secondary indexing won't change (except for the writing-index-updates-to-WAL bit). The transaction ID would be propagated to Puts for the index updates (cross RS or not) to prevent the possibility of getting into an inconsistent state. So we'd still have the potential for a deadlock if this isn't fixed. Otherwise, all the work you're doing to handle out-of-order updates would have to be done on the client instead of the RS which would require raw scans and returning a bunch of extra data to the client.

          Show
          jamestaylor James Taylor added a comment - - edited Jesse Yates - wrt transactions, the plumbing for secondary indexing won't change (except for the writing-index-updates-to-WAL bit). The transaction ID would be propagated to Puts for the index updates (cross RS or not) to prevent the possibility of getting into an inconsistent state. So we'd still have the potential for a deadlock if this isn't fixed. Otherwise, all the work you're doing to handle out-of-order updates would have to be done on the client instead of the RS which would require raw scans and returning a bunch of extra data to the client.
          Hide
          apurtell Andrew Purtell added a comment -

          HBase will amend the changes on HBASE-11497 to make the RpcScheduler an abstract class rather than an interface. Attached v3 patch for Phoenix master, tested with https://github.com/apurtell/hbase/tree/HBASE-11497-0.98 . Only changes 'implements' to 'extends' in PhoenixIndexRpcScheduler.java.

          Show
          apurtell Andrew Purtell added a comment - HBase will amend the changes on HBASE-11497 to make the RpcScheduler an abstract class rather than an interface. Attached v3 patch for Phoenix master, tested with https://github.com/apurtell/hbase/tree/HBASE-11497-0.98 . Only changes 'implements' to 'extends' in PhoenixIndexRpcScheduler.java.
          Hide
          jesse_yates Jesse Yates added a comment -

          So what happens when we run this on 0.98.3? Current version supports running on both 0.98.3 and .4... I'd like to keep that compatibility if we can, but missing the .3 point version isn't the end of the world... thoughts?

          Show
          jesse_yates Jesse Yates added a comment - So what happens when we run this on 0.98.3? Current version supports running on both 0.98.3 and .4... I'd like to keep that compatibility if we can, but missing the .3 point version isn't the end of the world... thoughts?
          Hide
          apurtell Andrew Purtell added a comment -

          Current version supports running on both 0.98.3 and .4... I'd like to keep that compatibility if we can, but missing the .3 point version isn't the end of the world... thoughts?

          Would be best to move to the LimitedPrivate interfaces we've set up to avoid future issues. Perhaps it would be enough to document the discontinuity at the 0.98.3 to 0.98.4 transition for Phoenix users? There are 84 issues fixed in .4, with 6 more pending, 7 of them blockers. How strongly do you feel about supporting 0.98 versions < 4? We could possibly synthesize the index RPC controller using ASM to avoid compilation issues and then instantiate it with reflection. A fair amount of work there.

          Show
          apurtell Andrew Purtell added a comment - Current version supports running on both 0.98.3 and .4... I'd like to keep that compatibility if we can, but missing the .3 point version isn't the end of the world... thoughts? Would be best to move to the LimitedPrivate interfaces we've set up to avoid future issues. Perhaps it would be enough to document the discontinuity at the 0.98.3 to 0.98.4 transition for Phoenix users? There are 84 issues fixed in .4, with 6 more pending, 7 of them blockers. How strongly do you feel about supporting 0.98 versions < 4? We could possibly synthesize the index RPC controller using ASM to avoid compilation issues and then instantiate it with reflection. A fair amount of work there.
          Hide
          jesse_yates Jesse Yates added a comment - - edited

          Well, as its a new addition, support 0.98.4 as the first version doesn't seem like that big of an issue - basically, "add these configs and roll your cluster with the new hbase + phoenix".

          And it would be nice to drop all the 0.98.3 specific code of out of the patch (which creates its own RpcExecutor, reflection, etc). Not worth all the extra work to make it work with the inheritance heirarchy

          Show
          jesse_yates Jesse Yates added a comment - - edited Well, as its a new addition, support 0.98.4 as the first version doesn't seem like that big of an issue - basically, "add these configs and roll your cluster with the new hbase + phoenix". And it would be nice to drop all the 0.98.3 specific code of out of the patch (which creates its own RpcExecutor, reflection, etc). Not worth all the extra work to make it work with the inheritance heirarchy
          Hide
          apurtell Andrew Purtell added a comment -

          Ok. If you think of anything else that needs to go in here before the .4 release is official, please let me know. We can pull the RC for further work if needed.

          Show
          apurtell Andrew Purtell added a comment - Ok. If you think of anything else that needs to go in here before the .4 release is official, please let me know. We can pull the RC for further work if needed.
          Hide
          jamestaylor James Taylor added a comment -

          Thanks for the patch. I suppose if the release of 0.98.4 slips past the end of the month (hopefully unlikely), then the commit can be reverted (as we need to do a Phoenix release by the end of the month).

          Show
          jamestaylor James Taylor added a comment - Thanks for the patch. I suppose if the release of 0.98.4 slips past the end of the month (hopefully unlikely), then the commit can be reverted (as we need to do a Phoenix release by the end of the month).
          Hide
          yuzhihong@gmail.com Ted Yu added a comment -

          + Class.forName("org.apache.hadoop.hbase.ipc.RpcExector");

          Looks like there was a typo above: RpcExector

          Show
          yuzhihong@gmail.com Ted Yu added a comment - + Class.forName("org.apache.hadoop.hbase.ipc.RpcExector"); Looks like there was a typo above: RpcExector
          Hide
          jesse_yates Jesse Yates added a comment -

          We are going to drop all that code anyways, since we are just going to support 0.98.4+ going forward. I'll work up a new patch.

          Show
          jesse_yates Jesse Yates added a comment - We are going to drop all that code anyways, since we are just going to support 0.98.4+ going forward. I'll work up a new patch.
          Hide
          jesse_yates Jesse Yates added a comment -

          Patch that just works on 0.98.4+. Some handling to throw some reasonable error messages in case you are running on the wrong version. Removes all the reflection/wrapping stuff to handle 0.98.3 as well

          Show
          jesse_yates Jesse Yates added a comment - Patch that just works on 0.98.4+. Some handling to throw some reasonable error messages in case you are running on the wrong version. Removes all the reflection/wrapping stuff to handle 0.98.3 as well
          Hide
          jesse_yates Jesse Yates added a comment -

          and linking this to HBASE-11513 so we an use the BalancedRpcExecutor in Phoenix as well, rather than having to make our own.

          Show
          jesse_yates Jesse Yates added a comment - and linking this to HBASE-11513 so we an use the BalancedRpcExecutor in Phoenix as well, rather than having to make our own.
          Hide
          jesse_yates Jesse Yates added a comment -

          Now that HBase 0.98.4 has been released (thanks again Andrew Purtell!), looks like its time to commit this guy too. I plan on committing tonight/early tomorrow, unless there are any objections.

          Show
          jesse_yates Jesse Yates added a comment - Now that HBase 0.98.4 has been released (thanks again Andrew Purtell !), looks like its time to commit this guy too. I plan on committing tonight/early tomorrow, unless there are any objections.
          Hide
          jamestaylor James Taylor added a comment -

          +1, as long as you've tested the new Phoenix snapshot jar against a pre-0.98.4 release to make sure we'll still run (albeit without your fix).

          Show
          jamestaylor James Taylor added a comment - +1, as long as you've tested the new Phoenix snapshot jar against a pre-0.98.4 release to make sure we'll still run (albeit without your fix).
          Hide
          apurtell Andrew Purtell added a comment -

          Confirmed that artifacts for 0.98.4-hadoop1 and 0.98.4-hadoop2 are available now on repository.apache.org

          Show
          apurtell Andrew Purtell added a comment - Confirmed that artifacts for 0.98.4-hadoop1 and 0.98.4-hadoop2 are available now on repository.apache.org
          Hide
          jesse_yates Jesse Yates added a comment -

          Attaching actually committed code for master and 4.0 branches. Had to make a slight change as the Cell interface changed in 0.98.4 by adding a method.

          Anyways, I think just adding the method implementation should be alright as older code won't call that method and the newer code will build just fine (since it will have the new method)

          Show
          jesse_yates Jesse Yates added a comment - Attaching actually committed code for master and 4.0 branches. Had to make a slight change as the Cell interface changed in 0.98.4 by adding a method. Anyways, I think just adding the method implementation should be alright as older code won't call that method and the newer code will build just fine (since it will have the new method)
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Phoenix | Master | Hadoop1 #289 (See https://builds.apache.org/job/Phoenix-master-hadoop1/289/)
          PHOENIX-938: Use higher priority queue for index updates to prevent deadlock (jyates: rev 1954c717a12561bdc2184ba23c53afae3f900084)

          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/builder/BaseIndexBuilder.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexQosRpcControllerFactory.java
          • phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexHandlerIT.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexQosCompat.java
          • phoenix-core/src/main/java/org/apache/phoenix/util/IndexUtil.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/table/CoprocessorHTableFactory.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/builder/IndexBuildManager.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/ipc/PhoenixIndexRpcSchedulerFactory.java
          • pom.xml
          • phoenix-core/src/main/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcScheduler.java
          • phoenix-core/src/test/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcSchedulerTest.java
          • phoenix-core/src/main/java/org/apache/phoenix/hbase/index/builder/IndexBuilder.java
          • phoenix-core/src/test/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactoryTest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Phoenix | Master | Hadoop1 #289 (See https://builds.apache.org/job/Phoenix-master-hadoop1/289/ ) PHOENIX-938 : Use higher priority queue for index updates to prevent deadlock (jyates: rev 1954c717a12561bdc2184ba23c53afae3f900084) phoenix-core/src/main/java/org/apache/phoenix/hbase/index/builder/BaseIndexBuilder.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexQosRpcControllerFactory.java phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexHandlerIT.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexQosCompat.java phoenix-core/src/main/java/org/apache/phoenix/util/IndexUtil.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/table/CoprocessorHTableFactory.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/builder/IndexBuildManager.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/ipc/PhoenixIndexRpcSchedulerFactory.java pom.xml phoenix-core/src/main/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcScheduler.java phoenix-core/src/test/java/org/apache/hadoop/hbase/ipc/PhoenixIndexRpcSchedulerTest.java phoenix-core/src/main/java/org/apache/phoenix/hbase/index/builder/IndexBuilder.java phoenix-core/src/test/java/org/apache/hadoop/hbase/regionserver/PhoenixIndexRpcSchedulerFactoryTest.java
          Hide
          jesse_yates Jesse Yates added a comment -

          There was a 4.0 failure here, but passed locally.... probably just a machine flake.

          James Taylor We also need to update the site with docs on turning this on (requires HBase configs) - let me know how I can help (happy to do it in the future if you point me to the site code).

          Show
          jesse_yates Jesse Yates added a comment - There was a 4.0 failure here , but passed locally.... probably just a machine flake. James Taylor We also need to update the site with docs on turning this on (requires HBase configs) - let me know how I can help (happy to do it in the future if you point me to the site code).
          Hide
          jamestaylor James Taylor added a comment -

          Jesse Yates - that'd be great if you could update the website docs. The website lives in SVN and in general is based on a set of markdown files. Here's the process:

          • checkout the website: svn checkout https://svn.apache.org/repos/asf/phoenix
          • modify the appropriate markdown file: ./site/source/src/site/markdown/secondary_indexing.md
          • generate the html: ./build.sh
          • checkin the modified files (including the generate html file(s)): svn commit

          mujtaba - would you mind adding this info to the How to Contribute page?

          Show
          jamestaylor James Taylor added a comment - Jesse Yates - that'd be great if you could update the website docs. The website lives in SVN and in general is based on a set of markdown files. Here's the process: checkout the website: svn checkout https://svn.apache.org/repos/asf/phoenix modify the appropriate markdown file: ./site/source/src/site/markdown/secondary_indexing.md generate the html: ./build.sh checkin the modified files (including the generate html file(s)): svn commit mujtaba - would you mind adding this info to the How to Contribute page?
          Hide
          Jeongdae Kim Jeongdae Kim added a comment - - edited

          Jesse Yates - I found something strange in your codes, that callExecutor.start() do not be called anywhere in PhoenixIndexRpcScheduler as below.
          @Override
          public void start()

          { delegate.start(); }

          i think callExecutor.start() must be called in PhoenixIndexRpcScheduler.start() as below to work fine as you intended.

          @Override
          public void start()

          { delegate.start(); callExecutor.start(port); }

          could you explain your intention or some codes that i might not be seen before, if your implement is intended?

          Show
          Jeongdae Kim Jeongdae Kim added a comment - - edited Jesse Yates - I found something strange in your codes, that callExecutor.start() do not be called anywhere in PhoenixIndexRpcScheduler as below. @Override public void start() { delegate.start(); } i think callExecutor.start() must be called in PhoenixIndexRpcScheduler.start() as below to work fine as you intended. @Override public void start() { delegate.start(); callExecutor.start(port); } could you explain your intention or some codes that i might not be seen before, if your implement is intended?
          Hide
          jesse_yates Jesse Yates added a comment -

          Yup, that's a bug.

          Over on PHOENIX-1676 we are tracking down all the issues with the index priority - mind adding your comment over there?

          At some point I was trying to refactor HBase Rpc Schedulers to handle generic queues so scheduler impls wouldn't have to actually manage their own queues, but alas, that started to get very convoluted and was never finished.

          Show
          jesse_yates Jesse Yates added a comment - Yup, that's a bug. Over on PHOENIX-1676 we are tracking down all the issues with the index priority - mind adding your comment over there? At some point I was trying to refactor HBase Rpc Schedulers to handle generic queues so scheduler impls wouldn't have to actually manage their own queues, but alas, that started to get very convoluted and was never finished.
          Hide
          enis Enis Soztutar added a comment -

          Bulk close of all issues that has been resolved in a released version.

          Show
          enis Enis Soztutar added a comment - Bulk close of all issues that has been resolved in a released version.

            People

            • Assignee:
              jesse_yates Jesse Yates
              Reporter:
              jamestaylor James Taylor
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development