bharatviswa504 commented on a change in pull request #2141:
URL: https://github.com/apache/ozone/pull/2141#discussion_r618297230
##########
File path:
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAUtils.java
##########
@@ -198,12 +201,40 @@ public static OzoneConfiguration removeSelfId(
return getSCMNodeIds(configuration, scmServiceId);
}
+ private static Throwable unwrapException(Exception e) {
+ IOException ioException = null;
+ Throwable cause = e.getCause();
Review comment:
should we start here from e, not e.getCause()?
##########
File path:
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAUtils.java
##########
@@ -198,12 +201,40 @@ 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;
+ }
+
+ /**
+ * Checks if the underlying exception if of type StateMachine. Used by scm
+ * clients.
+ */
+ public static boolean isNonRetriableException(Exception e) {
+ Throwable t =
+ getExceptionForClass(e, StateMachineException.class);
+ return t == null ? false : true;
+ }
+
+ /**
+ * Checks if the underlying exception if of type non retriable. Used by scm
+ * clients.
+ */
+ public static boolean checkNonRetriableException(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);
Review comment:
Should be hereThrowable t = e, instead of unwrapException?
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMContainerLocationFailoverProxyProvider.java
##########
@@ -154,13 +162,26 @@ public synchronized void performFailover(
LOG.debug("Failing over to next proxy. {}", getCurrentProxySCMNodeId());
}
- public synchronized void performFailoverToAssignedLeader(String newLeader) {
+ public void performFailoverToAssignedLeader(String newLeader, Exception e) {
Review comment:
I think we should have synchronized here.
Same for all classses
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMBlockLocationFailoverProxyProvider.java
##########
@@ -180,7 +200,7 @@ private synchronized long getRetryInterval() {
return retryInterval;
}
- private synchronized int nextProxyIndex() {
+ private int nextProxyIndex() {
Review comment:
Why synchronized is removed here?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1098,6 +1098,16 @@ public String getClientRpcPort() {
return addr == null ? "0" : Integer.toString(addr.getPort());
}
+ public String getBlockProtocolRpcPort() {
+ InetSocketAddress addr = getBlockProtocolServer().getBlockRpcAddress();
+ return addr == null ? "0" : Integer.toString(addr.getPort());
+ }
+
+ public String getSecurityProtocolRpcPort() {
+ InetSocketAddress addr = getSecurityProtocolServer().getRpcAddress();
+ return addr == null ? "0" : Integer.toString(addr.getPort());
Review comment:
Same here?
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -1098,6 +1098,16 @@ public String getClientRpcPort() {
return addr == null ? "0" : Integer.toString(addr.getPort());
}
+ public String getBlockProtocolRpcPort() {
+ InetSocketAddress addr = getBlockProtocolServer().getBlockRpcAddress();
+ return addr == null ? "0" : Integer.toString(addr.getPort());
Review comment:
Do we need these nulll check?
##########
File path:
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMContainerLocationFailoverProxyProvider.java
##########
@@ -131,6 +132,13 @@ public synchronized String getCurrentProxySCMNodeId() {
return currentProxySCMNodeId;
}
+ @VisibleForTesting
+ public void changeCurrentProxy(String nodeId) {
Review comment:
Synchronized needed here?
--
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]
