You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/03/03 21:26:13 UTC
[GitHub] [ozone] kerneltime opened a new pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
kerneltime opened a new pull request #3160:
URL: https://github.com/apache/ozone/pull/3160
## What changes were proposed in this pull request?
Perform failover in the correct interface that is guaranteed to be called only once in a multi-threaded environment.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5228
## How was this patch tested?
- [ ] Existing tests
- [ ] Manual testing
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #3160:
URL: https://github.com/apache/ozone/pull/3160#issuecomment-1068544205
@neils-dev @hanishakoneru this PR is ready for the final review.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
Posted by GitBox <gi...@apache.org>.
neils-dev commented on pull request #3160:
URL: https://github.com/apache/ozone/pull/3160#issuecomment-1069675688
Thanks @kerneltime for this. LGTM.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] neils-dev commented on a change in pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #3160:
URL: https://github.com/apache/ozone/pull/3160#discussion_r822229881
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##########
@@ -339,39 +341,27 @@ protected Text computeDelegationTokenService() {
/**
* Called whenever an error warrants failing over. It is determined by the
- * retry policy.
- *
- * This is a dummy call from {@link RetryInvocationHandler}. The actual
- * failover should be performed using either
- * {@link OMFailoverProxyProvider#performFailoverIfRequired(String)} or
- * {@link OMFailoverProxyProvider#performFailoverToNextProxy()}.
- *
- * In {@link OzoneManagerProtocolClientSideTranslatorPB}, we first
- * manually failover and then call the RetryAction FAILOVER_AND_RETRY. This
- * is done because we do not want to always failover to the next proxy. If we
- * get a OMNotLeaderException with a suggested leader, then we want to
- * failover to that OM proxy instead. Hence, we failover manually and the
- * {@link FailoverProxyProvider#performFailover(Object)} call should not do
- * failover again.
+ * retry policy. This method is supposed to called only once in a
+ * multithreaded environment. This where the fail over occurs.
*/
Review comment:
Not important - cosmetic, above comment reads _fail over_ two separate words, instead of the action `failover`
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##########
@@ -261,9 +263,9 @@ public RetryAction shouldRetry(Exception exception, int retries,
// address of the suggested leader along with the nodeID.
// Failing over just based on nodeID is not very robust.
- // OMFailoverProxyProvider#performFailover() is a dummy call and
- // does not perform any failover. Failover manually to the next OM.
- performFailoverToNextProxy();
+ // Prepare the next OM to be tried. This will help with calculation
+ // of the wait times needed get creating the retryAction.
+ selectNextOmProxy();
Review comment:
With multiple threads, in the case when the `nextOmProxyIndex` is set by one thread calling `shouldRetry` and another thread executes `shouldRetry` prior to the `performFailover` (setting of the` currentOmNodeId`), the _nextOmProxyIndex_ could be incremented before the OmProxy is tried. Can this be a problem? Should we not check if the _nextOmProxyIndex_ == `currentOmProxyIndex` before trying the next proxy (calling `selectNextOmProxy`), some sort of synchronized check?
If `nextOmProxyIndex` != `currentOmProxyIndex `(unsynchronized), then it implies another thread is in failover and has just executed `shouldRetry` prior to calling `performFailover`. We need to try the current `nextOmProxyIndex` before trying other.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##########
@@ -115,8 +115,10 @@ public OMFailoverProxyProvider(ConfigurationSource configuration,
loadOMClientConfigs(conf, this.omServiceId);
Review comment:
@kerneltime Thanks for doing this patch and bringing up this concurrency issue in our PR for HDDS-5544 support for OM HA. Following up on the note to avoid using `shouldRetry` for failover and instead use `performFailover` as implemented here, I found the following comment useful from the scm failover provider code,
_// performFailOver() updates the currentProxyOmNodeId
// When 2 or more threads run in parallel, the
// RetryInvocationHandler will check the expectedFailOverCount
// and not execute performFailOver() for one of them. So the other thread(s)
// shall not call performFailOver(), instead it will call getProxy()._
It would be great to add a similar comment here in the code as well.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on a change in pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3160:
URL: https://github.com/apache/ozone/pull/3160#discussion_r823203520
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##########
@@ -261,9 +263,9 @@ public RetryAction shouldRetry(Exception exception, int retries,
// address of the suggested leader along with the nodeID.
// Failing over just based on nodeID is not very robust.
- // OMFailoverProxyProvider#performFailover() is a dummy call and
- // does not perform any failover. Failover manually to the next OM.
- performFailoverToNextProxy();
+ // Prepare the next OM to be tried. This will help with calculation
+ // of the wait times needed get creating the retryAction.
+ selectNextOmProxy();
Review comment:
Agree.
Even though performFailover will be called only once by concurrent failed invocations (guaranteed by RetryInvocationHandler), this code does not guarantee that the failover will occur to the next proxy. The failover could as well happen to the same proxy if we do not ensure that selectNextOmProxy is effective only once for concurrent failovers (same as before).
Having nextOmProxyId == currentOmProxyId check would help here.
The reason for not using the RetryInvocationHandler#performFailover() to do the actual failover was that with the base method we could not failover to the suggested leader OM (which we are anyway not doing currently but it would be good to add it back).
I am thinking if we should update the attemptedOMs list (used to calculate waitTime) from performFailover instead of from selectNextOmProxy.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru merged pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
Posted by GitBox <gi...@apache.org>.
hanishakoneru merged pull request #3160:
URL: https://github.com/apache/ozone/pull/3160
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on a change in pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #3160:
URL: https://github.com/apache/ozone/pull/3160#discussion_r823203520
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##########
@@ -261,9 +263,9 @@ public RetryAction shouldRetry(Exception exception, int retries,
// address of the suggested leader along with the nodeID.
// Failing over just based on nodeID is not very robust.
- // OMFailoverProxyProvider#performFailover() is a dummy call and
- // does not perform any failover. Failover manually to the next OM.
- performFailoverToNextProxy();
+ // Prepare the next OM to be tried. This will help with calculation
+ // of the wait times needed get creating the retryAction.
+ selectNextOmProxy();
Review comment:
Agree.
Even though performFailover will be called only once by concurrent failed invocations (guaranteed by RetryInvocationHandler), this code does not guarantee that the failover will occur to the next proxy. The failover could as well happen to the same proxy if we do not ensure that selectNextOmProxy is effective only once for concurrent failovers (same as before).
Having nextOmProxyId == currentOmProxyId check would help here.
The reason for not using the RetryInvocationHandler#performFailover() to do the actual failover was that with the base method we could not failover to the suggested leader OM (which we are anyway not doing currently but it would be good to add it back).
Would it be better to update the suggested leaderOM from shouldRetry and make the decision to failover to the suggested LeaderOM or the next proxy from performFailover? It would reduce the complexity of using synchronized methods in OmFailoverProxyProvider. The attemptedOMs list (used to calculate waitTime) can also be updated from performFailover. Have to think about the implications for this once again.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3160:
URL: https://github.com/apache/ozone/pull/3160#discussion_r824904491
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##########
@@ -115,8 +115,10 @@ public OMFailoverProxyProvider(ConfigurationSource configuration,
loadOMClientConfigs(conf, this.omServiceId);
Review comment:
Done. Added to `performFailover()`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3160: HDDS-5228 Make OM FailOverProxyProvider work across threads
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3160:
URL: https://github.com/apache/ozone/pull/3160#discussion_r823218644
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##########
@@ -261,9 +263,9 @@ public RetryAction shouldRetry(Exception exception, int retries,
// address of the suggested leader along with the nodeID.
// Failing over just based on nodeID is not very robust.
- // OMFailoverProxyProvider#performFailover() is a dummy call and
- // does not perform any failover. Failover manually to the next OM.
- performFailoverToNextProxy();
+ // Prepare the next OM to be tried. This will help with calculation
+ // of the wait times needed get creating the retryAction.
+ selectNextOmProxy();
Review comment:
Agreed. I will make a slightly different change and update.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org