Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: ha, ipc
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Implementing client failover will likely require changes to o.a.h.io.ipc and/or o.a.h.io.retry. This JIRA is to track those changes.

      1. hadoop-7380.0.patch
        27 kB
        Aaron T. Myers
      2. hadoop-7380.1.patch
        33 kB
        Aaron T. Myers
      3. hadoop-7380.2.patch
        35 kB
        Aaron T. Myers
      4. hadoop-7380-hdfs-example.patch
        4 kB
        Aaron T. Myers
      5. hdfs-7380.3.patch
        37 kB
        Aaron T. Myers

        Issue Links

          Activity

          Hide
          Aaron T. Myers added a comment -

          Here's a first hack at the Common portion of client failover, not intended for commit.

          If people think this approach is roughly sound I'll fix it up and prepare it for commit. It still needs at least the following work:

          • A way to mark certain interface methods as idempotent (and thus should be retried upon failover). This will probably take the form of a method annotation.
          • Refinement of the precise set of exceptions to deal with.
          • Potential refinement of the FailoverProxyProvider interface. I'm not in love with it as it stands.

          I've also created a test which uses this code to actually perform a failover between two federated NNs (which happen to have the same FS metadata) by starting them both up, creating a file in each at the same path, shutting down one, and ensuring the DFSClient properly fails over to the second. I can post that patch on HDFS-1973 but it depends on this patch.

          Show
          Aaron T. Myers added a comment - Here's a first hack at the Common portion of client failover, not intended for commit. If people think this approach is roughly sound I'll fix it up and prepare it for commit. It still needs at least the following work: A way to mark certain interface methods as idempotent (and thus should be retried upon failover). This will probably take the form of a method annotation. Refinement of the precise set of exceptions to deal with. Potential refinement of the FailoverProxyProvider interface. I'm not in love with it as it stands. I've also created a test which uses this code to actually perform a failover between two federated NNs (which happen to have the same FS metadata) by starting them both up, creating a file in each at the same path, shutting down one, and ensuring the DFSClient properly fails over to the second. I can post that patch on HDFS-1973 but it depends on this patch.
          Hide
          Aaron T. Myers added a comment -

          Updated patch which adds support for marking interface methods as being idempotent. Again, not intended for commit just yet.

          I also updated the DFSClient failover test case to ensure it works with this annotation facility, which it does.

          Show
          Aaron T. Myers added a comment - Updated patch which adds support for marking interface methods as being idempotent. Again, not intended for commit just yet. I also updated the DFSClient failover test case to ensure it works with this annotation facility, which it does.
          Hide
          Aaron T. Myers added a comment -

          Another open issue which needs to be addressed is synchronization around the FailoverProxyProvider on failover. This will be necessary since multiple threads may be accessing a single RPC proxy, and we don't want this to effectively cause the client to failover multiple times spuriously.

          I'll deal with this once I get confirmation that the general approach is sound.

          Show
          Aaron T. Myers added a comment - Another open issue which needs to be addressed is synchronization around the FailoverProxyProvider on failover. This will be necessary since multiple threads may be accessing a single RPC proxy, and we don't want this to effectively cause the client to failover multiple times spuriously. I'll deal with this once I get confirmation that the general approach is sound.
          Hide
          Eli Collins added a comment -

          Took a quick look at the patch. Overall the approach looks sane, I'll follow up with more detailed review feedback.

          Show
          Eli Collins added a comment - Took a quick look at the patch. Overall the approach looks sane, I'll follow up with more detailed review feedback.
          Hide
          Aaron T. Myers added a comment -

          Here's an updated patch which I believe is ready for commit. I mostly just rebased against trunk and added comments since the last one.

          Eli, please let me know what you think.

          Show
          Aaron T. Myers added a comment - Here's an updated patch which I believe is ready for commit. I mostly just rebased against trunk and added comments since the last one. Eli, please let me know what you think.
          Hide
          Eli Collins added a comment -

          Apologies for the delay, please post the patch you mention above wrt using this to fail over between federated NNs to HDFS-1973. Useful in the context of looking at this patch.

          Show
          Eli Collins added a comment - Apologies for the delay, please post the patch you mention above wrt using this to fail over between federated NNs to HDFS-1973 . Useful in the context of looking at this patch.
          Hide
          Eli Collins added a comment -

          Latest patch looks good.

          The following is implicit in the change, but I think it's worth stating here in the jira explicitly: in FailoverOnNetworkExceptionRetry#shouldRetry we don't fail-over and retry if we're making a non-idempotent call and there's an IOException or SocketException that's not Connect, NoRouteToHost, UnknownHost, or Standby. The rationale of course is that the operation may have reached the server and retrying elsewhere could leave us in an insconsistent state. This means if a client doing a create/delete which gets a SocketTimeoutException (which is an IOE) or an EOF SocketException the exception will be thrown all the way up to the caller of FileSystem/FileContext. That's reasonable because only the user of the API at this level has sufficient knoweldge of how to handle the failure, eg if they get such an exception after issuing a delete they can check if the file still exists and if so re-issue the delete (however they may also not want to do this, and FileContext doesn't know which).

          Minor comments:

          • Need to mark the new Interfaces with @InterfaceStability.Evolving
          • The new create methods in RetryProxy need javadocs (or you could move the javadoc to your new methods and add the FailoverProxyProvider param if you feel we're OD'ing on javadocs here)
          • @param lines shouldn't end with a period (eg RetryPolicy and FailOverProxyProvider)
          • Should RetryAction be in RetryPolicy or do you expect there to be a class here eventually?
          • Wonder if the LOG.info in RetryInvocationFailure for the fail-over case should be a warning.

          I think this change is sufficiently decoupled from HDFS-1973 that we can check it into trunk before we branch for HA.

          Show
          Eli Collins added a comment - Latest patch looks good. The following is implicit in the change, but I think it's worth stating here in the jira explicitly: in FailoverOnNetworkExceptionRetry#shouldRetry we don't fail-over and retry if we're making a non-idempotent call and there's an IOException or SocketException that's not Connect, NoRouteToHost, UnknownHost, or Standby. The rationale of course is that the operation may have reached the server and retrying elsewhere could leave us in an insconsistent state. This means if a client doing a create/delete which gets a SocketTimeoutException (which is an IOE) or an EOF SocketException the exception will be thrown all the way up to the caller of FileSystem/FileContext. That's reasonable because only the user of the API at this level has sufficient knoweldge of how to handle the failure, eg if they get such an exception after issuing a delete they can check if the file still exists and if so re-issue the delete (however they may also not want to do this, and FileContext doesn't know which). Minor comments: Need to mark the new Interfaces with @InterfaceStability.Evolving The new create methods in RetryProxy need javadocs (or you could move the javadoc to your new methods and add the FailoverProxyProvider param if you feel we're OD'ing on javadocs here) @param lines shouldn't end with a period (eg RetryPolicy and FailOverProxyProvider) Should RetryAction be in RetryPolicy or do you expect there to be a class here eventually? Wonder if the LOG.info in RetryInvocationFailure for the fail-over case should be a warning. I think this change is sufficiently decoupled from HDFS-1973 that we can check it into trunk before we branch for HA.
          Hide
          Eli Collins added a comment -

          In the first paragraph of the previous comment I meant to state that we may not want to retry a given rpc because the user needs to retry a higher-level operation which may consist of multiple rpcs.

          Show
          Eli Collins added a comment - In the first paragraph of the previous comment I meant to state that we may not want to retry a given rpc because the user needs to retry a higher-level operation which may consist of multiple rpcs.
          Hide
          Aaron T. Myers added a comment -

          please post the patch you mention above wrt using this to fail over between federated NNs to HDFS-1973. Useful in the context of looking at this patch.

          Here ya go. (Not intended for commit.)

          Show
          Aaron T. Myers added a comment - please post the patch you mention above wrt using this to fail over between federated NNs to HDFS-1973 . Useful in the context of looking at this patch. Here ya go. (Not intended for commit.)
          Hide
          Aaron T. Myers added a comment -

          Updating description of this JIRA since, as Eli suggests, it is a discrete change and will likely go in independently of HDFS-1973.

          Show
          Aaron T. Myers added a comment - Updating description of this JIRA since, as Eli suggests, it is a discrete change and will likely go in independently of HDFS-1973 .
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review, Eli. Here's a patch which addresses all of them.

          Show
          Aaron T. Myers added a comment - Thanks a lot for the review, Eli. Here's a patch which addresses all of them.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12485646/hdfs-7380.3.patch
          against trunk revision 1143681.

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

          +1 tests included. The patch appears to include 8 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 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 core tests. The patch passed core unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/710//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/710//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/710//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/12485646/hdfs-7380.3.patch against trunk revision 1143681. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 8 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 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 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/710//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/710//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/710//console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          +1 looks great

          Show
          Eli Collins added a comment - +1 looks great
          Hide
          Eli Collins added a comment -

          I've committed this. Thanks atm.

          Show
          Eli Collins added a comment - I've committed this. Thanks atm.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #681 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/681/)
          HADOOP-7380. Add client failover functionality to o.a.h.io.(ipc|retry). Contributed by Aaron T. Myers

          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1144043
          Files :

          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryPolicy.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryProxy.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryPolicies.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/TestFailoverProxy.java
          • /hadoop/common/trunk/common/CHANGES.txt
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/ipc/StandbyException.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/UnreliableInterface.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/DefaultFailoverProxyProvider.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/UnreliableImplementation.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/Idempotent.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #681 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/681/ ) HADOOP-7380 . Add client failover functionality to o.a.h.io.(ipc|retry). Contributed by Aaron T. Myers eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1144043 Files : /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryPolicy.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryProxy.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryPolicies.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/trunk/common/CHANGES.txt /hadoop/common/trunk/common/src/java/org/apache/hadoop/ipc/StandbyException.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/UnreliableInterface.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/DefaultFailoverProxyProvider.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/Idempotent.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #741 (See https://builds.apache.org/job/Hadoop-Common-trunk/741/)
          HADOOP-7380. Add client failover functionality to o.a.h.io.(ipc|retry). Contributed by Aaron T. Myers

          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1144043
          Files :

          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryPolicy.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryProxy.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryPolicies.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/TestFailoverProxy.java
          • /hadoop/common/trunk/common/CHANGES.txt
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/ipc/StandbyException.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/UnreliableInterface.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/DefaultFailoverProxyProvider.java
          • /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/UnreliableImplementation.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java
          • /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/Idempotent.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #741 (See https://builds.apache.org/job/Hadoop-Common-trunk/741/ ) HADOOP-7380 . Add client failover functionality to o.a.h.io.(ipc|retry). Contributed by Aaron T. Myers eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1144043 Files : /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryPolicy.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryProxy.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryPolicies.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/trunk/common/CHANGES.txt /hadoop/common/trunk/common/src/java/org/apache/hadoop/ipc/StandbyException.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/UnreliableInterface.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/DefaultFailoverProxyProvider.java /hadoop/common/trunk/common/src/test/core/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java /hadoop/common/trunk/common/src/java/org/apache/hadoop/io/retry/Idempotent.java
          Hide
          Suresh Srinivas added a comment -

          Aaron, should this change have been made in HA branch. Also it would be good to add a little design document. This is a fundamental change in the IPC layer. I have concerns on adding failover, retry etc. all in one patch. It would help reviews if this is done in a smaller patches.

          Show
          Suresh Srinivas added a comment - Aaron, should this change have been made in HA branch. Also it would be good to add a little design document. This is a fundamental change in the IPC layer. I have concerns on adding failover, retry etc. all in one patch. It would help reviews if this is done in a smaller patches.
          Hide
          Sanjay Radia added a comment -

          I am also worried about this patch - it needs a design document.

          Show
          Sanjay Radia added a comment - I am also worried about this patch - it needs a design document.
          Hide
          Aaron T. Myers added a comment -

          Aaron, should this change have been made in HA branch.

          Eli seemed to think this change was sufficiently generic that it could go into trunk before the HA branch was cut. I'm inclined to agree with him, but if you feel strongly, we could certainly back it out of trunk and commit it only to the HA branch. I don't see much benefit in doing so versus just addressing any concerns you have in subsequent patches, but I don't feel strongly about that.

          Also it would be good to add a little design document.

          Sure, I can work on that.

          This is a fundamental change in the IPC layer. I have concerns on adding failover, retry etc. all in one patch.

          I think you're overstating the extent of this change a little bit. This patch doesn't introduce retry - all of that was already in place. This patch augments the existing retry facility to optionally fail over and retry on a different proxy object.

          It would help reviews if this is done in a smaller patches.

          Certainly, I can do that in the future.

          Show
          Aaron T. Myers added a comment - Aaron, should this change have been made in HA branch. Eli seemed to think this change was sufficiently generic that it could go into trunk before the HA branch was cut. I'm inclined to agree with him, but if you feel strongly, we could certainly back it out of trunk and commit it only to the HA branch. I don't see much benefit in doing so versus just addressing any concerns you have in subsequent patches, but I don't feel strongly about that. Also it would be good to add a little design document. Sure, I can work on that. This is a fundamental change in the IPC layer. I have concerns on adding failover, retry etc. all in one patch. I think you're overstating the extent of this change a little bit. This patch doesn't introduce retry - all of that was already in place. This patch augments the existing retry facility to optionally fail over and retry on a different proxy object. It would help reviews if this is done in a smaller patches. Certainly, I can do that in the future.
          Hide
          Eli Collins added a comment -

          Sanjay, is there something specific in the patch you're worried about?

          Show
          Eli Collins added a comment - Sanjay, is there something specific in the patch you're worried about?
          Hide
          Eli Collins added a comment -

          Btw this is just the IPC plumbing for retry, fail-over is covered in HDFS-1973.

          Show
          Eli Collins added a comment - Btw this is just the IPC plumbing for retry, fail-over is covered in HDFS-1973 .
          Hide
          Sanjay Radia added a comment -

          I am concerned about the idempotent operation part of the patch.
          This is a significant change and it would be good to describe and discuss this.
          Aaron could you please provide a short description in this Jira.

          Also shouldn't this be going into the HA branch?
          As we use it with NN failover this will need to be refined. Further it will require a test plan that we can develop a part of the HA branch.

          Show
          Sanjay Radia added a comment - I am concerned about the idempotent operation part of the patch. This is a significant change and it would be good to describe and discuss this. Aaron could you please provide a short description in this Jira. Also shouldn't this be going into the HA branch? As we use it with NN failover this will need to be refined. Further it will require a test plan that we can develop a part of the HA branch.
          Hide
          Aaron T. Myers added a comment -

          I am concerned about the idempotent operation part of the patch.
          This is a significant change and it would be good to describe and discuss this.
          Aaron could you please provide a short description in this Jira.

          Sure, I'll write something up.

          Also shouldn't this be going into the HA branch?

          As I've already said:

          Eli seemed to think this change was sufficiently generic that it could go into trunk before the HA branch was cut. I'm inclined to agree with him, but if you feel strongly, we could certainly back it out of trunk and commit it only to the HA branch. I don't see much benefit in doing so versus just addressing any concerns you have in subsequent patches, but I don't feel strongly about that.

          Please let me know if you want me to do that.

          As we use it with NN failover this will need to be refined.

          I'm sure it will. Do you have any specific changes in mind? If so, I'd be happy to file follow-up JIRAs to address them.

          Further it will require a test plan that we can develop a part of the HA branch.

          A test plan for just this patch beyond the tests which were included as part of this patch? What are you looking for out of said test plan? I certainly agree that we should come up with a test plan for the entirety of the HA work, but I don't think that has much to do with this specific patch.

          Show
          Aaron T. Myers added a comment - I am concerned about the idempotent operation part of the patch. This is a significant change and it would be good to describe and discuss this. Aaron could you please provide a short description in this Jira. Sure, I'll write something up. Also shouldn't this be going into the HA branch? As I've already said: Eli seemed to think this change was sufficiently generic that it could go into trunk before the HA branch was cut. I'm inclined to agree with him, but if you feel strongly, we could certainly back it out of trunk and commit it only to the HA branch. I don't see much benefit in doing so versus just addressing any concerns you have in subsequent patches, but I don't feel strongly about that. Please let me know if you want me to do that. As we use it with NN failover this will need to be refined. I'm sure it will. Do you have any specific changes in mind? If so, I'd be happy to file follow-up JIRAs to address them. Further it will require a test plan that we can develop a part of the HA branch. A test plan for just this patch beyond the tests which were included as part of this patch? What are you looking for out of said test plan? I certainly agree that we should come up with a test plan for the entirety of the HA work, but I don't think that has much to do with this specific patch.
          Hide
          Sanjay Radia added a comment -
          • Yes, lets put it in the HA branch.
          • I don't have specific changes in mind but I would like to connect this up to DNS, ZK and IP based fail-over and see how
            apis meet the needs. The branch is a good place to evolve it. Further as we plug the NN rpc interface it will help us
            validate the idempotent attributed added by this jira.
          • A client fail-over will require testing in a realistic network. This can be a part of the specific set of fail-over
            over mechanisms we add in the HA branch.
          Show
          Sanjay Radia added a comment - Yes, lets put it in the HA branch. I don't have specific changes in mind but I would like to connect this up to DNS, ZK and IP based fail-over and see how apis meet the needs. The branch is a good place to evolve it. Further as we plug the NN rpc interface it will help us validate the idempotent attributed added by this jira. A client fail-over will require testing in a realistic network. This can be a part of the specific set of fail-over over mechanisms we add in the HA branch.
          Hide
          Aaron T. Myers added a comment -

          Yes, lets put it in the HA branch.

          To be clear, it's already in the HA branch by virtue of having been in trunk before the HA branch was created. Do you want me to revert it from trunk?

          I don't have specific changes in mind but I would like to connect this up to DNS, ZK and IP based fail-over and see how

          apis meet the needs. The branch is a good place to evolve it. Further as we plug the NN rpc interface it will help us
          validate the idempotent attributed added by this jira.

          I totally agree. I look forward to implementing those and seeing how this API evolves to suit.

          Show
          Aaron T. Myers added a comment - Yes, lets put it in the HA branch. To be clear, it's already in the HA branch by virtue of having been in trunk before the HA branch was created. Do you want me to revert it from trunk? I don't have specific changes in mind but I would like to connect this up to DNS, ZK and IP based fail-over and see how apis meet the needs. The branch is a good place to evolve it. Further as we plug the NN rpc interface it will help us validate the idempotent attributed added by this jira. I totally agree. I look forward to implementing those and seeing how this API evolves to suit.
          Hide
          Aaron T. Myers added a comment -

          As requested, here's a design doc:

          Client Retry/Fail Over in IPC Overview

          The goal of HDFS-1973 is to provide a facility for clients to fail over and retry an RPC to a different NN in the event of failure, in support of HDFS HA. Since the HDFS RPC mechanisms are built on top of the Common IPC library, this is a natural place to implement this functionality. Furthermore, since client fail over in general has challenges which are not HDFS-specific, we may be able to reuse this mechanism for other HA services in the future (e.g. perhaps the MRv2 Resource Manager.)

          Goals

          This mechanism should be able to support the following:

          1. A way to support multiple distinct ways for determining which object an RPC should be attempted against.
          2. A way of specifying customized failover strategies. For example, some HA service may only support a pair of machines to serve requests, while another might support an arbitrary number. The strategy for failing over to a new machine might be different for these cases.
          3. The method for specifying a retry/failover strategy should be able to control both retry and failover logic. For example, a strategy may want to try a request to server A once, attempt a failover to server B immediately, fail back to server A and then retry several times with some amount of backoff.
          4. A way for a remote process to indicate that it is not the appropriate process to serve the request, and that an attempt should be made to another process.
          5. A way of specifying that some operations in an IPC interface are not safe to be failed over and retried.

          Approach

          The Common IPC library already supports retrying an IPC against the same remote process. This is done via the classes in the o.a.h.io.retry package. The exact desired retry semantics can be specified using an implementation of the RetryPolicy interface. An implementation of this interface is responsible for determining whether or not a particular method call should be retried given the number of times its already been tried and the particular exception which caused the method to fail. An implementer of this interface can either choose to have the failed method retried, or the exception re-thrown.

          In a sense, client-side failover is exactly the same as the existing retry mechanism, except method calls can be retried against a different proxy object. Thus, this JIRA proposes to extend the existing retry facility to also support client failover.

          Presently, the RetryPolicy.shouldRetry method only returns a boolean to indicate whether a method invocation should be retried or considered to have failed. This can be augmented to return an enum value to indicate whether a particular method invocation should fail, be retried against the same object, or retried on another object. In order to determine what object a method should be tried against, this JIRA introduces the concept of a FailoverProxyProvider. An implementer of this interface is capable of getting a proxy object and initiating a client failover when the result of RetryPolicy.shouldRetry indicates a failover should be performed for the particular RetryPolicy the RetryInvocationHandler is configured to use. This addresses goals 1, 2, and 3 from above.

          To address goal 4, this JIRA introduces a new exception type - StandbyException - which can be thrown by remote processes to indicate that it is not the appropriate process to handle requests for a given service at this time. RetryPolicy implementations may choose to handle this exception differently than other exception types when determining whether or not to retry or fail over an operation.

          Though there may be circumstances in which a client may desire a more complex retry/failover strategy, most clients will want to failover only on network exceptions in which an RPC is guaranteed to have not reached the remote process, or in cases in which the particular method to be retried will have no mutative ill-effects if retried (e.g. read operations or idempotent write operations.) This JIRA thus introduces a new RetryPolicy implementation - FailoverOnNetworkExceptionRetry. This retry policy fails over whenever a method call fails in such a way as to guarantee that it did not reach the original remote process, or when retrying a method which can be safely retried. In order to know which methods of an IPC interface can be safely retried, this JIRA introduces an @Idempotent method annotation which, if present, will be passed on to the retry policy by the RetryInvocationHandler when determining whether or not to retry a method. This addresses goal 5.

          Show
          Aaron T. Myers added a comment - As requested, here's a design doc: Client Retry/Fail Over in IPC Overview The goal of HDFS-1973 is to provide a facility for clients to fail over and retry an RPC to a different NN in the event of failure, in support of HDFS HA. Since the HDFS RPC mechanisms are built on top of the Common IPC library, this is a natural place to implement this functionality. Furthermore, since client fail over in general has challenges which are not HDFS-specific, we may be able to reuse this mechanism for other HA services in the future (e.g. perhaps the MRv2 Resource Manager.) Goals This mechanism should be able to support the following: A way to support multiple distinct ways for determining which object an RPC should be attempted against. A way of specifying customized failover strategies. For example, some HA service may only support a pair of machines to serve requests, while another might support an arbitrary number. The strategy for failing over to a new machine might be different for these cases. The method for specifying a retry/failover strategy should be able to control both retry and failover logic. For example, a strategy may want to try a request to server A once, attempt a failover to server B immediately, fail back to server A and then retry several times with some amount of backoff. A way for a remote process to indicate that it is not the appropriate process to serve the request, and that an attempt should be made to another process. A way of specifying that some operations in an IPC interface are not safe to be failed over and retried. Approach The Common IPC library already supports retrying an IPC against the same remote process. This is done via the classes in the o.a.h.io.retry package. The exact desired retry semantics can be specified using an implementation of the RetryPolicy interface. An implementation of this interface is responsible for determining whether or not a particular method call should be retried given the number of times its already been tried and the particular exception which caused the method to fail. An implementer of this interface can either choose to have the failed method retried, or the exception re-thrown. In a sense, client-side failover is exactly the same as the existing retry mechanism, except method calls can be retried against a different proxy object. Thus, this JIRA proposes to extend the existing retry facility to also support client failover. Presently, the RetryPolicy.shouldRetry method only returns a boolean to indicate whether a method invocation should be retried or considered to have failed. This can be augmented to return an enum value to indicate whether a particular method invocation should fail, be retried against the same object, or retried on another object. In order to determine what object a method should be tried against, this JIRA introduces the concept of a FailoverProxyProvider . An implementer of this interface is capable of getting a proxy object and initiating a client failover when the result of RetryPolicy.shouldRetry indicates a failover should be performed for the particular RetryPolicy the RetryInvocationHandler is configured to use. This addresses goals 1, 2, and 3 from above. To address goal 4, this JIRA introduces a new exception type - StandbyException - which can be thrown by remote processes to indicate that it is not the appropriate process to handle requests for a given service at this time. RetryPolicy implementations may choose to handle this exception differently than other exception types when determining whether or not to retry or fail over an operation. Though there may be circumstances in which a client may desire a more complex retry/failover strategy, most clients will want to failover only on network exceptions in which an RPC is guaranteed to have not reached the remote process, or in cases in which the particular method to be retried will have no mutative ill-effects if retried (e.g. read operations or idempotent write operations.) This JIRA thus introduces a new RetryPolicy implementation - FailoverOnNetworkExceptionRetry . This retry policy fails over whenever a method call fails in such a way as to guarantee that it did not reach the original remote process, or when retrying a method which can be safely retried. In order to know which methods of an IPC interface can be safely retried, this JIRA introduces an @Idempotent method annotation which, if present, will be passed on to the retry policy by the RetryInvocationHandler when determining whether or not to retry a method. This addresses goal 5.
          Hide
          Sanjay Radia added a comment -

          Thanks for the doc.

          • Any suggestions for an alternate name to StandbyException - the IPC layer should not know about Standby or Active. Perhaps "RedirectException"?
          • Clearly one cannot the server to always respond with an exception (it won't in a split brain situation).
            Looks good otherwise. Please move to HA branch.
          Show
          Sanjay Radia added a comment - Thanks for the doc. Any suggestions for an alternate name to StandbyException - the IPC layer should not know about Standby or Active. Perhaps "RedirectException"? Clearly one cannot the server to always respond with an exception (it won't in a split brain situation). Looks good otherwise. Please move to HA branch.

            People

            • Assignee:
              Aaron T. Myers
              Reporter:
              Aaron T. Myers
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development