Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-11395

RequestHedgingProxyProvider#RequestHedgingInvocationHandler hides the Exception thrown from NameNode

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-alpha4, 2.8.2
    • Component/s: ha
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When using RequestHedgingProxyProvider, in case of Exception (like FileNotFoundException) from ActiveNameNode, RequestHedgingProxyProvider#RequestHedgingInvocationHandler.invoke receives ExecutionException since we use CompletionService for the call. The ExecutionException is put into a map and wrapped with MultiException.

      So for a FileNotFoundException the client receives MultiException(Map(ExecutionException(InvocationTargetException(RemoteException(FileNotFoundException)))))

      It will cause problem in clients which are handling RemoteExceptions.

      1. HDFS-11395.000.patch
        5 kB
        Nanda kumar
      2. HDFS-11395.001.patch
        8 kB
        Nanda kumar
      3. HDFS-11395.002.patch
        8 kB
        Nanda kumar
      4. HDFS-11395.003.patch
        11 kB
        Nanda kumar
      5. HDFS-11395.004.patch
        11 kB
        Nanda kumar
      6. HDFS-11395.005.patch
        11 kB
        Nanda kumar

        Issue Links

          Activity

          Hide
          nandakumar131 Nanda kumar added a comment -

          If we get RemoteException other than StandbyException, it is safe to throw it rather than wrapping it into MultiException.

          Arpit Agarwal any comment or suggestion on this?

          Show
          nandakumar131 Nanda kumar added a comment - If we get RemoteException other than StandbyException , it is safe to throw it rather than wrapping it into MultiException . Arpit Agarwal any comment or suggestion on this?
          Hide
          nandakumar131 Nanda kumar added a comment -

          In HDFS-11395.000.patch, if RequestHedgingProxyProvider#RequestHedgingInvocationHandler.invoke receives a remote exception which is not a StandbyException, it is directly thrown to the caller.

          Show
          nandakumar131 Nanda kumar added a comment - In HDFS-11395.000.patch , if RequestHedgingProxyProvider#RequestHedgingInvocationHandler.invoke receives a remote exception which is not a StandbyException , it is directly thrown to the caller.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 13m 22s trunk passed
          +1 compile 0m 46s trunk passed
          +1 checkstyle 0m 35s trunk passed
          +1 mvnsite 0m 51s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 44s trunk passed
          +1 javadoc 0m 40s trunk passed
          +1 mvninstall 0m 47s the patch passed
          +1 compile 0m 42s the patch passed
          +1 javac 0m 42s the patch passed
          +1 checkstyle 0m 31s the patch passed
          +1 mvnsite 0m 47s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 49s the patch passed
          +1 javadoc 0m 37s the patch passed
          -1 unit 91m 25s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          116m 54s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency
            hadoop.hdfs.TestRollingUpgrade



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11395
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12852351/HDFS-11395.000.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c0da78168b42 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 172b23a
          Default Java 1.8.0_121
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18402/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18402/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18402/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 13m 22s trunk passed +1 compile 0m 46s trunk passed +1 checkstyle 0m 35s trunk passed +1 mvnsite 0m 51s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 44s trunk passed +1 javadoc 0m 40s trunk passed +1 mvninstall 0m 47s the patch passed +1 compile 0m 42s the patch passed +1 javac 0m 42s the patch passed +1 checkstyle 0m 31s the patch passed +1 mvnsite 0m 47s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 49s the patch passed +1 javadoc 0m 37s the patch passed -1 unit 91m 25s hadoop-hdfs in the patch failed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 116m 54s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency   hadoop.hdfs.TestRollingUpgrade Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11395 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12852351/HDFS-11395.000.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c0da78168b42 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 172b23a Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/18402/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18402/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18402/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          arpitagarwal Arpit Agarwal added a comment -

          Thanks for reporting this [~nanda131].

          Short-circuiting the execution looks wrong.

                      RemoteException rEx = getRemoteException(ex);
                      if(rEx !=null && !isStandbyException(ex)) {
                        throw rEx;
                      }
          

          However your change to unwrap the RemoteException is correct. What happens if you throw the unwrapped RemoteExceptions via MultiException i.e. throw MultiException(RemoteException(Exception1), RemoteException(Exception2))?

          Show
          arpitagarwal Arpit Agarwal added a comment - Thanks for reporting this [~nanda131] . Short-circuiting the execution looks wrong. RemoteException rEx = getRemoteException(ex); if (rEx != null && !isStandbyException(ex)) { throw rEx; } However your change to unwrap the RemoteException is correct. What happens if you throw the unwrapped RemoteExceptions via MultiException i.e. throw MultiException(RemoteException(Exception1), RemoteException(Exception2)) ?
          Hide
          nandakumar131 Nanda kumar added a comment -

          Thanks for the review Arpit Agarwal.

          The HDFS clients expect unwrapped remote exception (like FileNotFoundException, AccessControlException, UnresolvedPathException).
          Unwrapping of remote exception is done in DFSClient, DFSOutputStream, etc..

          For example

          DFSClient#callGetBlockLocations
          static LocatedBlocks callGetBlockLocations(ClientProtocol namenode,
                String src, long start, long length) 
                throws IOException {
              try {
                return namenode.getBlockLocations(src, start, length);
              } catch(RemoteException re) {
                throw re.unwrapRemoteException(AccessControlException.class,
                                               FileNotFoundException.class,
                                               UnresolvedPathException.class);
              }
          

          If we throw MultiException(RemoteException(Exception1), RemoteException(Exception2)) from RequestHedgingInvocationHandler, unwrapping will not be done (by DFSClient, DFSOutputStream, etc.. the catch block doesn't handle MultiException) and the hdfs clients will directly receive MultiException.

          The hdfs clients (like hbase, hive, user applications) have no idea on how to handle MultiException.

          Show
          nandakumar131 Nanda kumar added a comment - Thanks for the review Arpit Agarwal . The HDFS clients expect unwrapped remote exception (like FileNotFoundException, AccessControlException, UnresolvedPathException). Unwrapping of remote exception is done in DFSClient , DFSOutputStream , etc.. For example DFSClient#callGetBlockLocations static LocatedBlocks callGetBlockLocations(ClientProtocol namenode, String src, long start, long length) throws IOException { try { return namenode.getBlockLocations(src, start, length); } catch (RemoteException re) { throw re.unwrapRemoteException(AccessControlException.class, FileNotFoundException.class, UnresolvedPathException.class); } If we throw MultiException(RemoteException(Exception1), RemoteException(Exception2)) from RequestHedgingInvocationHandler, unwrapping will not be done (by DFSClient, DFSOutputStream, etc.. the catch block doesn't handle MultiException) and the hdfs clients will directly receive MultiException. The hdfs clients (like hbase, hive, user applications) have no idea on how to handle MultiException.
          Hide
          arpitagarwal Arpit Agarwal added a comment -

          Hi Nanda kumar, RetryInvocationHandler has some logic for unwrapping MultiException (code here). I haven't looked at it carefully so don't know if it does the right thing. Deciding what to do when we get a MultiException should probably be handled in the RetryInvocationHandler.

          The reason the short-circuit failure path looks incorrect is because it can cause spurious request failures if the standby thinks it is active and throws something other than StandbyException.

          Show
          arpitagarwal Arpit Agarwal added a comment - Hi Nanda kumar , RetryInvocationHandler has some logic for unwrapping MultiException ( code here ). I haven't looked at it carefully so don't know if it does the right thing. Deciding what to do when we get a MultiException should probably be handled in the RetryInvocationHandler. The reason the short-circuit failure path looks incorrect is because it can cause spurious request failures if the standby thinks it is active and throws something other than StandbyException.
          Hide
          nandakumar131 Nanda kumar added a comment -

          Deciding what to do when we get a MultiException should probably be handled in the RetryInvocationHandler.
          The reason the short-circuit failure path looks incorrect is because it can cause spurious request failures if the standby thinks it is active and throws something other than StandbyException.

          Yeah, true. Short-circuit failure path in RequestHedgingInvocationHandler is not a proper solution here.

          I had a look at RetryInvocationHandler, MultiException is handled only while constructing RetryInfo in RetryInvocationHandler#RetryInfo#newRetryInfo.
          In RetryInvocationHandler#handleException if retryInfo.fail != null the received exception is thrown as it is, which in our case is MultiException(ExecutionException(RemoteException(Exception)))

          As you suggested, we can unwrap ExecutionException and throw MultiException(RemoteException(Exception)) from RequestHedgingInvocationHandler.
          We still have to handle MultiException in RetryInvocationHandler.

          In RetryInvocationHandler, we will get list of RemoteExceptions' from MultiException, from which we can exclude StandbyExceptions'. Is there any other check that can be performed to identify the proper exception to throw back to client?

          Show
          nandakumar131 Nanda kumar added a comment - Deciding what to do when we get a MultiException should probably be handled in the RetryInvocationHandler. The reason the short-circuit failure path looks incorrect is because it can cause spurious request failures if the standby thinks it is active and throws something other than StandbyException. Yeah, true. Short-circuit failure path in RequestHedgingInvocationHandler is not a proper solution here. I had a look at RetryInvocationHandler, MultiException is handled only while constructing RetryInfo in RetryInvocationHandler#RetryInfo#newRetryInfo. In RetryInvocationHandler#handleException if retryInfo.fail != null the received exception is thrown as it is, which in our case is MultiException(ExecutionException(RemoteException(Exception))) As you suggested, we can unwrap ExecutionException and throw MultiException(RemoteException(Exception)) from RequestHedgingInvocationHandler. We still have to handle MultiException in RetryInvocationHandler. In RetryInvocationHandler, we will get list of RemoteExceptions' from MultiException, from which we can exclude StandbyExceptions'. Is there any other check that can be performed to identify the proper exception to throw back to client?
          Hide
          arpitagarwal Arpit Agarwal added a comment -

          Is there any other check that can be performed to identify the proper exception to throw back to client?

          Yes we need to decide which exception to throw to the client if the failure is decided to be non-retriable.

          We want to retain the exception other than StandbyException. If we have more than one such exception (should be rare), it may be okay to throw the first one in the list (needs more thought).

          If all NameNodes are standby then we should retain the same behavior we have today with ConfiguredFailoverProxyProvider.

          Show
          arpitagarwal Arpit Agarwal added a comment - Is there any other check that can be performed to identify the proper exception to throw back to client? Yes we need to decide which exception to throw to the client if the failure is decided to be non-retriable. We want to retain the exception other than StandbyException. If we have more than one such exception (should be rare), it may be okay to throw the first one in the list (needs more thought). If all NameNodes are standby then we should retain the same behavior we have today with ConfiguredFailoverProxyProvider.
          Hide
          nandakumar131 Nanda kumar added a comment -

          Thanks for the suggestion Arpit Agarwal, I will work on the patch.

          If we have more than one such exception (should be rare), it may be okay to throw the first one in the list (needs more thought).

          Here we have to handle the case where standby thinks it's active and throws something other than StandbyException. (Least we can do is to log all the exceptions, and then throw the first one)

          Show
          nandakumar131 Nanda kumar added a comment - Thanks for the suggestion Arpit Agarwal , I will work on the patch. If we have more than one such exception (should be rare), it may be okay to throw the first one in the list (needs more thought). Here we have to handle the case where standby thinks it's active and throws something other than StandbyException. (Least we can do is to log all the exceptions, and then throw the first one)
          Hide
          nandakumar131 Nanda kumar added a comment -

          Arpit Agarwal, based on the suggestions/comments new patch is uploaded - HDFS-11395.001.patch. Please review it.

          Show
          nandakumar131 Nanda kumar added a comment - Arpit Agarwal , based on the suggestions/comments new patch is uploaded - HDFS-11395.001.patch . Please review it.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 0m 15s Maven dependency ordering for branch
          +1 mvninstall 13m 0s trunk passed
          +1 compile 17m 45s trunk passed
          +1 checkstyle 2m 14s trunk passed
          +1 mvnsite 2m 39s trunk passed
          +1 mvneclipse 0m 39s trunk passed
          +1 findbugs 3m 45s trunk passed
          +1 javadoc 1m 52s trunk passed
          0 mvndep 0m 18s Maven dependency ordering for patch
          +1 mvninstall 2m 0s the patch passed
          +1 compile 16m 51s the patch passed
          +1 javac 16m 51s the patch passed
          +1 checkstyle 2m 23s the patch passed
          +1 mvnsite 2m 44s the patch passed
          +1 mvneclipse 0m 53s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 4m 50s the patch passed
          +1 javadoc 2m 6s the patch passed
          -1 unit 10m 18s hadoop-common in the patch failed.
          -1 unit 94m 36s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          181m 27s



          Reason Tests
          Failed junit tests hadoop.http.TestHttpServer
            hadoop.metrics2.sink.TestRollingFileSystemSinkWithHdfs



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11395
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855187/HDFS-11395.001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 122e87ba20a4 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 480b4dd
          Default Java 1.8.0_121
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18469/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18469/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18469/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18469/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 0m 15s Maven dependency ordering for branch +1 mvninstall 13m 0s trunk passed +1 compile 17m 45s trunk passed +1 checkstyle 2m 14s trunk passed +1 mvnsite 2m 39s trunk passed +1 mvneclipse 0m 39s trunk passed +1 findbugs 3m 45s trunk passed +1 javadoc 1m 52s trunk passed 0 mvndep 0m 18s Maven dependency ordering for patch +1 mvninstall 2m 0s the patch passed +1 compile 16m 51s the patch passed +1 javac 16m 51s the patch passed +1 checkstyle 2m 23s the patch passed +1 mvnsite 2m 44s the patch passed +1 mvneclipse 0m 53s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 4m 50s the patch passed +1 javadoc 2m 6s the patch passed -1 unit 10m 18s hadoop-common in the patch failed. -1 unit 94m 36s hadoop-hdfs in the patch failed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 181m 27s Reason Tests Failed junit tests hadoop.http.TestHttpServer   hadoop.metrics2.sink.TestRollingFileSystemSinkWithHdfs Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11395 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855187/HDFS-11395.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 122e87ba20a4 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 480b4dd Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/18469/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18469/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18469/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18469/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jingzhao Jing Zhao added a comment -

          Thanks for working on this, Nanda kumar. I agree we should not directly throw a MultiException. But I have similar concern as Arpit, i.e., we should not simply throw the first exception. I think we should

          1. Not mix detailed exception handling logic into RequestHedgingProxyProvider. In RequestHedgingProxyProvider, we only need to get the RemoteException from ExecutionException, and put all the exceptions into badResults. No need for special handling for StandbyException etc there. These should be handled by RetryInvocationHandler#newRetryInfo.
          2. Then in RetryInvocationHandler#newRetryInfo, we should let this method return both the RetryInfo and the exception to throw from the MultiException. These two information should comes from the same internal exception inside of the MultiException.
          Show
          jingzhao Jing Zhao added a comment - Thanks for working on this, Nanda kumar . I agree we should not directly throw a MultiException. But I have similar concern as Arpit, i.e., we should not simply throw the first exception. I think we should Not mix detailed exception handling logic into RequestHedgingProxyProvider . In RequestHedgingProxyProvider , we only need to get the RemoteException from ExecutionException , and put all the exceptions into badResults . No need for special handling for StandbyException etc there. These should be handled by RetryInvocationHandler#newRetryInfo . Then in RetryInvocationHandler#newRetryInfo , we should let this method return both the RetryInfo and the exception to throw from the MultiException. These two information should comes from the same internal exception inside of the MultiException.
          Hide
          nandakumar131 Nanda kumar added a comment -

          Thanks for the review Jing Zhao.

          I agree, we should not simply throw the first exception. It will be helpful if you can provide some input on

          Not mix detailed exception handling logic into RequestHedgingProxyProvider

          True, but in case of non RemoteException from ExecutionException, what should be done?

          Then in RetryInvocationHandler#newRetryInfo, we should let this method return both the RetryInfo and the exception to throw from the MultiException.

          In that case, is it ok to add an additional field to RetryInvocationHandler#RetryInfo for holding Exception when RetryInfo.action == RetryAction.RetryDecision.FAIL ?

          Show
          nandakumar131 Nanda kumar added a comment - Thanks for the review Jing Zhao . I agree, we should not simply throw the first exception. It will be helpful if you can provide some input on Not mix detailed exception handling logic into RequestHedgingProxyProvider True, but in case of non RemoteException from ExecutionException, what should be done? Then in RetryInvocationHandler#newRetryInfo, we should let this method return both the RetryInfo and the exception to throw from the MultiException. In that case, is it ok to add an additional field to RetryInvocationHandler#RetryInfo for holding Exception when RetryInfo.action == RetryAction.RetryDecision.FAIL ?
          Hide
          jingzhao Jing Zhao added a comment -

          in case of non RemoteException from ExecutionException, what should be done

          That should be OK. The current retry policies are supposed to handle all exceptions including non RemoteException. E.g., FailoverOnNetworkExceptionRetry handles ConnectException, EOFException etc.

          In that case, is it ok to add an additional field to RetryInvocationHandler#RetryInfo for holding Exception

          Yes, sounds good to me.

          Show
          jingzhao Jing Zhao added a comment - in case of non RemoteException from ExecutionException, what should be done That should be OK. The current retry policies are supposed to handle all exceptions including non RemoteException. E.g., FailoverOnNetworkExceptionRetry handles ConnectException , EOFException etc. In that case, is it ok to add an additional field to RetryInvocationHandler#RetryInfo for holding Exception Yes, sounds good to me.
          Hide
          nandakumar131 Nanda kumar added a comment -

          In HDFS-11395.002.patch, RequestHedgingProxyProvider#RequestHedgingInvocationHandler gets RemoteException from ExecutionException, and puts all the exception into badResults and throws MultiException(badResults).

          RetryInvocationHandler#RetryInfo now has a new field to hold Exception, which can be used to throw in case of failure.

          Please review the patch.

          Show
          nandakumar131 Nanda kumar added a comment - In HDFS-11395.002.patch , RequestHedgingProxyProvider#RequestHedgingInvocationHandler gets RemoteException from ExecutionException, and puts all the exception into badResults and throws MultiException(badResults). RetryInvocationHandler#RetryInfo now has a new field to hold Exception, which can be used to throw in case of failure. Please review the patch.
          Hide
          jingzhao Jing Zhao added a comment -

          Thanks for updating the patch, Nanda kumar. Some further comments on the latest patch:

          1. In RetryAction, the usage of the new member field exception is for FAIL case. Thus maybe we can:
            • rename "exception" to failException
            • assign a value to this field only when the action is FAIL:
              +      Exception ex = null;
              
          2. Looks like we should not only get RemoteException out of ex, but more generally get the cause of other types of exceptions. Note exceptions like ConnectException and EOFException should also be exposed to retry policies.
                        RemoteException rEx = getRemoteException(ex);
                        if(rEx != null) {
                          badResults.put(tProxyInfo.proxyInfo, rEx);
                        } else {
                          badResults.put(tProxyInfo.proxyInfo, ex);
                        }
            
          3. It will be helpful if we can also have a unit test for the above ConnectException/EOFException case.
          4. Need to fix indentation, line length etc. for testHedgingWhenFileNotFoundException.
          Show
          jingzhao Jing Zhao added a comment - Thanks for updating the patch, Nanda kumar . Some further comments on the latest patch: In RetryAction, the usage of the new member field exception is for FAIL case. Thus maybe we can: rename "exception" to failException assign a value to this field only when the action is FAIL: + Exception ex = null ; Looks like we should not only get RemoteException out of ex , but more generally get the cause of other types of exceptions. Note exceptions like ConnectException and EOFException should also be exposed to retry policies. RemoteException rEx = getRemoteException(ex); if (rEx != null ) { badResults.put(tProxyInfo.proxyInfo, rEx); } else { badResults.put(tProxyInfo.proxyInfo, ex); } It will be helpful if we can also have a unit test for the above ConnectException/EOFException case. Need to fix indentation, line length etc. for testHedgingWhenFileNotFoundException .
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 24s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 0m 14s Maven dependency ordering for branch
          +1 mvninstall 12m 35s trunk passed
          -1 compile 9m 45s root in trunk failed.
          +1 checkstyle 1m 53s trunk passed
          +1 mvnsite 1m 59s trunk passed
          +1 mvneclipse 0m 38s trunk passed
          +1 findbugs 3m 14s trunk passed
          +1 javadoc 1m 34s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 24s the patch passed
          -1 compile 7m 53s root in the patch failed.
          -1 javac 7m 53s root in the patch failed.
          -0 checkstyle 2m 2s root: The patch generated 26 new + 9 unchanged - 0 fixed = 35 total (was 9)
          +1 mvnsite 2m 13s the patch passed
          +1 mvneclipse 0m 36s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 3m 41s the patch passed
          +1 javadoc 1m 33s the patch passed
          +1 unit 9m 32s hadoop-common in the patch passed.
          -1 unit 127m 34s hadoop-hdfs in the patch failed.
          +1 asflicense 1m 11s The patch does not generate ASF License warnings.
          191m 18s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.checker.TestThrottledAsyncChecker
            hadoop.hdfs.TestReadStripedFileWithMissingBlocks
            hadoop.hdfs.server.namenode.ha.TestHAAppend
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting
            hadoop.hdfs.server.blockmanagement.TestUnderReplicatedBlocks
          Timed out junit tests org.apache.hadoop.hdfs.TestLeaseRecovery2



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11395
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855926/HDFS-11395.002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 6f4b1c8a706a 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 490abfb
          Default Java 1.8.0_121
          compile https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/branch-compile-root.txt
          findbugs v3.0.0
          compile https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/patch-compile-root.txt
          javac https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/patch-compile-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18541/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18541/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 24s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 0m 14s Maven dependency ordering for branch +1 mvninstall 12m 35s trunk passed -1 compile 9m 45s root in trunk failed. +1 checkstyle 1m 53s trunk passed +1 mvnsite 1m 59s trunk passed +1 mvneclipse 0m 38s trunk passed +1 findbugs 3m 14s trunk passed +1 javadoc 1m 34s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 24s the patch passed -1 compile 7m 53s root in the patch failed. -1 javac 7m 53s root in the patch failed. -0 checkstyle 2m 2s root: The patch generated 26 new + 9 unchanged - 0 fixed = 35 total (was 9) +1 mvnsite 2m 13s the patch passed +1 mvneclipse 0m 36s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 3m 41s the patch passed +1 javadoc 1m 33s the patch passed +1 unit 9m 32s hadoop-common in the patch passed. -1 unit 127m 34s hadoop-hdfs in the patch failed. +1 asflicense 1m 11s The patch does not generate ASF License warnings. 191m 18s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.checker.TestThrottledAsyncChecker   hadoop.hdfs.TestReadStripedFileWithMissingBlocks   hadoop.hdfs.server.namenode.ha.TestHAAppend   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting   hadoop.hdfs.server.blockmanagement.TestUnderReplicatedBlocks Timed out junit tests org.apache.hadoop.hdfs.TestLeaseRecovery2 Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11395 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12855926/HDFS-11395.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 6f4b1c8a706a 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 490abfb Default Java 1.8.0_121 compile https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/branch-compile-root.txt findbugs v3.0.0 compile https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/patch-compile-root.txt javac https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/patch-compile-root.txt checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18541/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18541/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18541/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          nandakumar131 Nanda kumar added a comment -

          Thanks for the review Jing Zhao.

          The following review comments are incorporated in the latest patch: HDFS-11395.003.patch

          • Renaming of "exception" to "failException"
          • Assigning value to "failException" field only when the action is FAIL
          • Unwrapping of exception in RequestHedgingProxyProvider#RequestHedgingInvocationHandler#invoke, for all kinds of exception
          • Unit test case added to cover ConnectException/EOFException
          • Fixed indentation, line length in test cases

          Please review the new patch.

          Show
          nandakumar131 Nanda kumar added a comment - Thanks for the review Jing Zhao . The following review comments are incorporated in the latest patch: HDFS-11395.003.patch Renaming of "exception" to "failException" Assigning value to "failException" field only when the action is FAIL Unwrapping of exception in RequestHedgingProxyProvider#RequestHedgingInvocationHandler#invoke , for all kinds of exception Unit test case added to cover ConnectException/EOFException Fixed indentation, line length in test cases Please review the new patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 0m 18s Maven dependency ordering for branch
          +1 mvninstall 16m 3s trunk passed
          +1 compile 13m 15s trunk passed
          +1 checkstyle 2m 9s trunk passed
          +1 mvnsite 2m 27s trunk passed
          +1 mvneclipse 0m 53s trunk passed
          +1 findbugs 3m 56s trunk passed
          +1 javadoc 1m 53s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 31s the patch passed
          +1 compile 10m 59s the patch passed
          +1 javac 10m 59s the patch passed
          -0 checkstyle 2m 8s root: The patch generated 10 new + 9 unchanged - 0 fixed = 19 total (was 9)
          +1 mvnsite 2m 17s the patch passed
          +1 mvneclipse 0m 52s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 4m 4s the patch passed
          +1 javadoc 1m 54s the patch passed
          +1 unit 8m 31s hadoop-common in the patch passed.
          -1 unit 76m 1s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 50s The patch does not generate ASF License warnings.
          152m 12s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.TestDirectoryScanner
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11395
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856065/HDFS-11395.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 2b6cc79e3205 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / c8bd8ac
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18561/artifact/patchprocess/diff-checkstyle-root.txt
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18561/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18561/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18561/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 0m 18s Maven dependency ordering for branch +1 mvninstall 16m 3s trunk passed +1 compile 13m 15s trunk passed +1 checkstyle 2m 9s trunk passed +1 mvnsite 2m 27s trunk passed +1 mvneclipse 0m 53s trunk passed +1 findbugs 3m 56s trunk passed +1 javadoc 1m 53s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 31s the patch passed +1 compile 10m 59s the patch passed +1 javac 10m 59s the patch passed -0 checkstyle 2m 8s root: The patch generated 10 new + 9 unchanged - 0 fixed = 19 total (was 9) +1 mvnsite 2m 17s the patch passed +1 mvneclipse 0m 52s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 4m 4s the patch passed +1 javadoc 1m 54s the patch passed +1 unit 8m 31s hadoop-common in the patch passed. -1 unit 76m 1s hadoop-hdfs in the patch failed. +1 asflicense 0m 50s The patch does not generate ASF License warnings. 152m 12s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDirectoryScanner   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11395 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856065/HDFS-11395.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 2b6cc79e3205 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2 15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c8bd8ac Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18561/artifact/patchprocess/diff-checkstyle-root.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/18561/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18561/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18561/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          nandakumar131 Nanda kumar added a comment -

          HDFS-11395.004.patch: checkstyle corrected.

          Show
          nandakumar131 Nanda kumar added a comment - HDFS-11395.004.patch : checkstyle corrected.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 41s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 2m 0s Maven dependency ordering for branch
          +1 mvninstall 14m 30s trunk passed
          +1 compile 11m 56s trunk passed
          +1 checkstyle 2m 22s trunk passed
          +1 mvnsite 2m 41s trunk passed
          +1 mvneclipse 0m 57s trunk passed
          +1 findbugs 4m 5s trunk passed
          +1 javadoc 1m 42s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 35s the patch passed
          +1 compile 11m 0s the patch passed
          +1 javac 11m 0s the patch passed
          -0 checkstyle 2m 5s root: The patch generated 10 new + 10 unchanged - 0 fixed = 20 total (was 10)
          +1 mvnsite 2m 43s the patch passed
          +1 mvneclipse 1m 1s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 4m 25s the patch passed
          +1 javadoc 2m 1s the patch passed
          +1 unit 8m 32s hadoop-common in the patch passed.
          +1 unit 95m 6s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 45s The patch does not generate ASF License warnings.
          171m 58s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11395
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856068/HDFS-11395.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 7f66a8e5847e 3.13.0-108-generic #155-Ubuntu SMP Wed Jan 11 16:58:52 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / c8bd8ac
          Default Java 1.8.0_121
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18562/artifact/patchprocess/diff-checkstyle-root.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18562/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18562/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 41s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 2m 0s Maven dependency ordering for branch +1 mvninstall 14m 30s trunk passed +1 compile 11m 56s trunk passed +1 checkstyle 2m 22s trunk passed +1 mvnsite 2m 41s trunk passed +1 mvneclipse 0m 57s trunk passed +1 findbugs 4m 5s trunk passed +1 javadoc 1m 42s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 35s the patch passed +1 compile 11m 0s the patch passed +1 javac 11m 0s the patch passed -0 checkstyle 2m 5s root: The patch generated 10 new + 10 unchanged - 0 fixed = 20 total (was 10) +1 mvnsite 2m 43s the patch passed +1 mvneclipse 1m 1s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 4m 25s the patch passed +1 javadoc 2m 1s the patch passed +1 unit 8m 32s hadoop-common in the patch passed. +1 unit 95m 6s hadoop-hdfs in the patch passed. +1 asflicense 0m 45s The patch does not generate ASF License warnings. 171m 58s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11395 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856068/HDFS-11395.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7f66a8e5847e 3.13.0-108-generic #155-Ubuntu SMP Wed Jan 11 16:58:52 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c8bd8ac Default Java 1.8.0_121 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/18562/artifact/patchprocess/diff-checkstyle-root.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18562/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18562/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jingzhao Jing Zhao added a comment -

          The 004 patch looks good to me. Just some minors:

          1. Here since we're sure e is a MultiException, we can directly catch "MultiException e".
            380	    } catch (Exception e) {
            381	      for (Exception ex : ((MultiException)e).getExceptions().values()) {
            
          2. The following code can be simplified as "Assert.assertTrue("......", rEx instanceof StandbyException)"
                      if (rEx instanceof StandbyException) {
                        continue;
                      } else {
                        Assert.fail("Unexpected RemoteException: " + rEx.getMessage());
                      }
            
          3. "@param ex" can be removed.
               * @param ex
               * @return unwrapped exception
               */
              private Exception unwrapException(Exception ex) {
            
          Show
          jingzhao Jing Zhao added a comment - The 004 patch looks good to me. Just some minors: Here since we're sure e is a MultiException, we can directly catch "MultiException e". 380 } catch (Exception e) { 381 for (Exception ex : ((MultiException)e).getExceptions().values()) { The following code can be simplified as "Assert.assertTrue("......", rEx instanceof StandbyException)" if (rEx instanceof StandbyException) { continue ; } else { Assert.fail( "Unexpected RemoteException: " + rEx.getMessage()); } "@param ex" can be removed. * @param ex * @ return unwrapped exception */ private Exception unwrapException(Exception ex) {
          Hide
          nandakumar131 Nanda kumar added a comment -

          Thanks Jing Zhao, review comments have be incorporated in HDFS-11395.005.patch

          Show
          nandakumar131 Nanda kumar added a comment - Thanks Jing Zhao , review comments have be incorporated in HDFS-11395.005.patch
          Hide
          jingzhao Jing Zhao added a comment -

          The 005 patch looks good to me. Let me trigger the Jenkins.

          Show
          jingzhao Jing Zhao added a comment - The 005 patch looks good to me. Let me trigger the Jenkins.
          Hide
          jingzhao Jing Zhao added a comment -

          hmmm looks like the testing node has not got fixed yet...

          Show
          jingzhao Jing Zhao added a comment - hmmm looks like the testing node has not got fixed yet...
          Show
          arpitagarwal Arpit Agarwal added a comment - Kicked off another pre-commit run: https://builds.apache.org/view/PreCommit%20Builds/job/PreCommit-HDFS-Build/18664/
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 1m 53s Maven dependency ordering for branch
          +1 mvninstall 12m 55s trunk passed
          +1 compile 11m 29s trunk passed
          +1 checkstyle 1m 56s trunk passed
          +1 mvnsite 2m 13s trunk passed
          +1 mvneclipse 0m 47s trunk passed
          +1 findbugs 3m 40s trunk passed
          +1 javadoc 1m 49s trunk passed
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 1m 39s the patch passed
          +1 compile 12m 27s the patch passed
          +1 javac 12m 27s the patch passed
          +1 checkstyle 2m 22s the patch passed
          +1 mvnsite 2m 53s the patch passed
          +1 mvneclipse 1m 9s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 4m 34s the patch passed
          +1 javadoc 1m 56s the patch passed
          +1 unit 9m 42s hadoop-common in the patch passed.
          -1 unit 109m 39s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 47s The patch does not generate ASF License warnings.
          186m 11s



          Reason Tests
          Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure
            hadoop.hdfs.server.blockmanagement.TestReconstructStripedBlocksWithRackAwareness
            hadoop.hdfs.TestRollingUpgrade
            hadoop.hdfs.server.namenode.ha.TestHAAppend
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HDFS-11395
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856337/HDFS-11395.005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 4a08f0242079 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 822a74f
          Default Java 1.8.0_121
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/18664/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18664/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: .
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18664/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 1m 53s Maven dependency ordering for branch +1 mvninstall 12m 55s trunk passed +1 compile 11m 29s trunk passed +1 checkstyle 1m 56s trunk passed +1 mvnsite 2m 13s trunk passed +1 mvneclipse 0m 47s trunk passed +1 findbugs 3m 40s trunk passed +1 javadoc 1m 49s trunk passed 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 1m 39s the patch passed +1 compile 12m 27s the patch passed +1 javac 12m 27s the patch passed +1 checkstyle 2m 22s the patch passed +1 mvnsite 2m 53s the patch passed +1 mvneclipse 1m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 4m 34s the patch passed +1 javadoc 1m 56s the patch passed +1 unit 9m 42s hadoop-common in the patch passed. -1 unit 109m 39s hadoop-hdfs in the patch failed. +1 asflicense 0m 47s The patch does not generate ASF License warnings. 186m 11s Reason Tests Failed junit tests hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure   hadoop.hdfs.server.blockmanagement.TestReconstructStripedBlocksWithRackAwareness   hadoop.hdfs.TestRollingUpgrade   hadoop.hdfs.server.namenode.ha.TestHAAppend   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailureReporting Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HDFS-11395 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12856337/HDFS-11395.005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4a08f0242079 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 822a74f Default Java 1.8.0_121 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/18664/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/18664/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs U: . Console output https://builds.apache.org/job/PreCommit-HDFS-Build/18664/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          nandakumar131 Nanda kumar added a comment -

          Test case failures are not related to this patch.

          Show
          nandakumar131 Nanda kumar added a comment - Test case failures are not related to this patch.
          Hide
          arpitagarwal Arpit Agarwal added a comment -

          Jing Zhao, are you +1 for this patch?

          Show
          arpitagarwal Arpit Agarwal added a comment - Jing Zhao , are you +1 for this patch?
          Hide
          jingzhao Jing Zhao added a comment -

          +1. I will commit the patch shortly.

          Show
          jingzhao Jing Zhao added a comment - +1. I will commit the patch shortly.
          Hide
          jingzhao Jing Zhao added a comment -

          I've committed the patch. Thanks for the contribution, Nanda kumar! Thanks for the review, Arpit Agarwal!

          Show
          jingzhao Jing Zhao added a comment - I've committed the patch. Thanks for the contribution, Nanda kumar ! Thanks for the review, Arpit Agarwal !
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11397 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11397/)
          HDFS-11395. RequestHedgingProxyProvider#RequestHedgingInvocationHandler (jing9: rev 55796a0946f80a35055701a34379e374399009c5)

          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/RequestHedgingProxyProvider.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestRequestHedgingProxyProvider.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11397 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11397/ ) HDFS-11395 . RequestHedgingProxyProvider#RequestHedgingInvocationHandler (jing9: rev 55796a0946f80a35055701a34379e374399009c5) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/RequestHedgingProxyProvider.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestRequestHedgingProxyProvider.java
          Hide
          nandakumar131 Nanda kumar added a comment -

          Thanks Arpit Agarwal & Jing Zhao for the reviews and commit.

          Show
          nandakumar131 Nanda kumar added a comment - Thanks Arpit Agarwal & Jing Zhao for the reviews and commit.
          Hide
          andrew.wang Andrew Wang added a comment -

          I backported this to branch-2.8 to make another backport easier, looks like a good fix too.

          Show
          andrew.wang Andrew Wang added a comment - I backported this to branch-2.8 to make another backport easier, looks like a good fix too.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          2.8.1 became a security release. Moving fix-version to 2.8.2 after the fact.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - 2.8.1 became a security release. Moving fix-version to 2.8.2 after the fact.

            People

            • Assignee:
              nandakumar131 Nanda kumar
              Reporter:
              nandakumar131 Nanda kumar
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development