bharatviswa504 commented on a change in pull request #2141:
URL: https://github.com/apache/ozone/pull/2141#discussion_r617367866
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMClientConfig.java
##########
@@ -86,7 +98,16 @@ public void setRpcTimeOut(long timeOut) {
}
public int getRetryCount() {
- return retryCount;
+ long duration = getMaxRetryTimeout();
Review comment:
[Discussion Question] Do we need this config still? As we get this
value anyway from maxRetryTimeOut. (Only case we get now is when retryCount
obtained from maxRetryTimeOut is less.
I have a mixed view, in few cases, it will help, if the client wants to
retry more time than the value we obtain. Not completely sure do we want it or
avoid it.
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HddsServerUtil.java
##########
@@ -461,26 +459,8 @@ public static SCMSecurityProtocolClientSideTranslatorPB
getScmSecurityClient(
public static SCMSecurityProtocolClientSideTranslatorPB
Review comment:
We can totally remove this method now.
It is same as getScmSecurityClient
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/RatisUtil.java
##########
@@ -174,4 +178,18 @@ private static void setRaftSnapshotProperties(
conf.getRatisSnapshotThreshold());
}
+ public static void checkRatisException(IOException e, String port,
+ String scmId) throws ServiceException {
+ if (SCMHAUtils.isNonRetriableException(e)) {
Review comment:
Here on server, we check this for NonRetriableException. But I don't see
where we throw it.
##########
File path:
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAUtils.java
##########
@@ -229,6 +273,8 @@ public static boolean
isRetriableWithNoFailoverException(Exception e) {
} else {
return RetryPolicy.RetryAction.FAIL;
}
+ } else if (SCMHAUtils.isNonRetriableException(e)) {
+ return RetryPolicy.RetryAction.FAIL;
} else {
if (failovers < maxRetryCount) {
return new RetryPolicy.RetryAction(
Review comment:
Here do we need to do manually failover, as performFailOver is a dummy
method.
##########
File path:
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAUtils.java
##########
@@ -198,12 +201,27 @@ public static OzoneConfiguration removeSelfId(
return getSCMNodeIds(configuration, scmServiceId);
}
+ private static Throwable unwrapException(Exception e) {
+ IOException ioException = null;
+ Throwable cause = e.getCause();
+ if (cause instanceof RemoteException) {
+ ioException = ((RemoteException) cause).unwrapRemoteException();
+ }
+ return ioException == null ? e : ioException;
+ }
+
+ public static boolean isNonRetriableException(Exception e) {
+ Throwable t = unwrapException(e);
+ return NonRetriableException.class.isInstance(t);
+ }
+
// This will return the underlying exception after unwrapping
// the exception to see if it matches with expected exception
// list , returns true otherwise will return false.
public static boolean isRetriableWithNoFailoverException(Exception e) {
- Throwable t = e;
- while (t != null && t.getCause() != null) {
+
+ Throwable t = unwrapException(e);
+ while (t != null) {
for (Class<? extends Exception> clazz :
getRetriableWithNoFailoverExceptionList()) {
Review comment:
Here we can check RetriableWithNoFailoverException right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
