You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by GitBox <gi...@apache.org> on 2022/06/08 07:49:38 UTC
[GitHub] [cxf] kwin opened a new pull request, #954: CXF-8712 One-off issue with retries
kwin opened a new pull request, #954:
URL: https://github.com/apache/cxf/pull/954
Immediately switch to the next alternate (address or endpoint) if the
current one has already exceeded the maxNumberOfRetries
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] kwin commented on a diff in pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #954:
URL: https://github.com/apache/cxf/pull/954#discussion_r894225648
##########
rt/features/clustering/src/main/java/org/apache/cxf/clustering/RetryStrategy.java:
##########
@@ -42,7 +42,11 @@ public List<Endpoint> getAlternateEndpoints(Exchange exchange) {
}
protected <T> T getNextAlternate(List<T> alternates) {
- return stillTheSameAddress() ? alternates.get(0) : alternates.remove(0);
+ // is the amount of retries for the first alternate already exceeded?
+ if (!stillTheSameAddress()) {
+ alternates.remove(0);
+ }
+ return alternates.isEmpty() ? null : alternates.get(0);
Review Comment:
This won't work for me, as I actually use this strategy with only one endpoint, so I would always run into the edge case.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta merged pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
reta merged PR #954:
URL: https://github.com/apache/cxf/pull/954
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] coheigea commented on pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
coheigea commented on PR #954:
URL: https://github.com/apache/cxf/pull/954#issuecomment-1262170158
> > @reta What's the status of this PR?
>
> @coheigea It looks abandoned, in general we need to make changes in a few places to prevent NPEs from happening
Thanks @reta , in that case I removed the fix versions from the corresponding JIRA.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
reta commented on PR #954:
URL: https://github.com/apache/cxf/pull/954#issuecomment-1150480658
Thank you for the fix, @kwin , I think you are right, there is max retries + 1 attempts actually being made, we should better respect this setting.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
reta commented on PR #954:
URL: https://github.com/apache/cxf/pull/954#issuecomment-1262178890
> @reta What is necessary for this PR to be merged? Just [#954 (comment)](https://github.com/apache/cxf/pull/954#discussion_r895181297) or anything else?
@kwin just that, thank you
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] kwin commented on a diff in pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #954:
URL: https://github.com/apache/cxf/pull/954#discussion_r983664031
##########
rt/features/clustering/src/main/java/org/apache/cxf/clustering/RetryStrategy.java:
##########
@@ -42,7 +42,11 @@ public List<Endpoint> getAlternateEndpoints(Exchange exchange) {
}
protected <T> T getNextAlternate(List<T> alternates) {
- return stillTheSameAddress() ? alternates.get(0) : alternates.remove(0);
+ // is the amount of retries for the first alternate already exceeded?
+ if (!stillTheSameAddress()) {
+ alternates.remove(0);
+ }
+ return alternates.isEmpty() ? null : alternates.get(0);
Review Comment:
Done in https://github.com/apache/cxf/pull/954/commits/cfca0e575821f577a9034d33d9f15b1638d94be2.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] kwin commented on a diff in pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #954:
URL: https://github.com/apache/cxf/pull/954#discussion_r893073176
##########
rt/features/clustering/src/main/java/org/apache/cxf/clustering/RetryStrategy.java:
##########
@@ -42,7 +42,11 @@ public List<Endpoint> getAlternateEndpoints(Exchange exchange) {
}
protected <T> T getNextAlternate(List<T> alternates) {
- return stillTheSameAddress() ? alternates.get(0) : alternates.remove(0);
+ // is the amount of retries for the first alternate already exceeded?
+ if (!stillTheSameAddress()) {
+ alternates.remove(0);
+ }
+ return alternates.isEmpty() ? null : alternates.get(0);
Review Comment:
Any other idea how to fix without returning null? Or do we need to change the contract for of selectNextAlternate to allow null return values?
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a diff in pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
reta commented on code in PR #954:
URL: https://github.com/apache/cxf/pull/954#discussion_r895181297
##########
rt/features/clustering/src/main/java/org/apache/cxf/clustering/RetryStrategy.java:
##########
@@ -42,7 +42,11 @@ public List<Endpoint> getAlternateEndpoints(Exchange exchange) {
}
protected <T> T getNextAlternate(List<T> alternates) {
- return stillTheSameAddress() ? alternates.get(0) : alternates.remove(0);
+ // is the amount of retries for the first alternate already exceeded?
+ if (!stillTheSameAddress()) {
+ alternates.remove(0);
+ }
+ return alternates.isEmpty() ? null : alternates.get(0);
Review Comment:
@kwin for this particular issue (have not looked into CXF-8288) we should be able to keep SPI unchanged by documenting `getNextAlternate` could return `null` and making sure `AbstractStaticFailoverStrategy` could deal with `null` in `selectAlternateXxx` methods.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a diff in pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
reta commented on code in PR #954:
URL: https://github.com/apache/cxf/pull/954#discussion_r894954105
##########
rt/features/clustering/src/main/java/org/apache/cxf/clustering/RetryStrategy.java:
##########
@@ -42,7 +42,11 @@ public List<Endpoint> getAlternateEndpoints(Exchange exchange) {
}
protected <T> T getNextAlternate(List<T> alternates) {
- return stillTheSameAddress() ? alternates.get(0) : alternates.remove(0);
+ // is the amount of retries for the first alternate already exceeded?
+ if (!stillTheSameAddress()) {
+ alternates.remove(0);
+ }
+ return alternates.isEmpty() ? null : alternates.get(0);
Review Comment:
> This won't work for me, as I actually use this strategy with only one endpoint, so I would always run into the edge case.
Agree. Just a suggestion, in this case you actually have an easy workaround (not a solution, just temporary shortcut): reduce the number of max retries by 1. I will try to look for a better fix meantime, will update you shortly.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
reta commented on PR #954:
URL: https://github.com/apache/cxf/pull/954#issuecomment-1262168666
> @reta What's the status of this PR?
@coheigea It looks abandoned, in general we need to make changes in a few places to prevent NPEs from happening
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] kwin commented on pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
kwin commented on PR #954:
URL: https://github.com/apache/cxf/pull/954#issuecomment-1262172264
@reta What is necessary for this PR to be merged? Just https://github.com/apache/cxf/pull/954#discussion_r895181297 or anything else?
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a diff in pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
reta commented on code in PR #954:
URL: https://github.com/apache/cxf/pull/954#discussion_r893908048
##########
rt/features/clustering/src/main/java/org/apache/cxf/clustering/RetryStrategy.java:
##########
@@ -42,7 +42,11 @@ public List<Endpoint> getAlternateEndpoints(Exchange exchange) {
}
protected <T> T getNextAlternate(List<T> alternates) {
- return stillTheSameAddress() ? alternates.get(0) : alternates.remove(0);
+ // is the amount of retries for the first alternate already exceeded?
+ if (!stillTheSameAddress()) {
+ alternates.remove(0);
+ }
+ return alternates.isEmpty() ? null : alternates.get(0);
Review Comment:
This is a great question, the suggestion I have in mind is not 100% bullet proof but I would love to hear your opinion:
```
protected <T> T getNextAlternate(List<T> alternates) {
String lastUsedAddress = null;
// is the amount of retries for the first alternate already exceeded?
if (!stillTheSameAddress()) {
lastUsedAddress = alternates.remove(0);
}
return alternates.isEmpty() ? lastUsedAddress : alternates.get(0);
}
```
In this case we would only go off the rails when there are no more alternatives to try out. WDYT?
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a diff in pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
reta commented on code in PR #954:
URL: https://github.com/apache/cxf/pull/954#discussion_r892918144
##########
rt/features/clustering/src/main/java/org/apache/cxf/clustering/RetryStrategy.java:
##########
@@ -42,7 +42,11 @@ public List<Endpoint> getAlternateEndpoints(Exchange exchange) {
}
protected <T> T getNextAlternate(List<T> alternates) {
- return stillTheSameAddress() ? alternates.get(0) : alternates.remove(0);
+ // is the amount of retries for the first alternate already exceeded?
+ if (!stillTheSameAddress()) {
+ alternates.remove(0);
+ }
+ return alternates.isEmpty() ? null : alternates.get(0);
Review Comment:
This is dangerous one, `getNextAlternate()` is not supposed to return `null` and is calling for trouble [1], we should accommodate that.
[1] https://github.com/apache/cxf/blob/master/rt/features/clustering/src/main/java/org/apache/cxf/clustering/AbstractStaticFailoverStrategy.java#L113
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] kwin commented on a diff in pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
kwin commented on code in PR #954:
URL: https://github.com/apache/cxf/pull/954#discussion_r894988226
##########
rt/features/clustering/src/main/java/org/apache/cxf/clustering/RetryStrategy.java:
##########
@@ -42,7 +42,11 @@ public List<Endpoint> getAlternateEndpoints(Exchange exchange) {
}
protected <T> T getNextAlternate(List<T> alternates) {
- return stillTheSameAddress() ? alternates.get(0) : alternates.remove(0);
+ // is the amount of retries for the first alternate already exceeded?
+ if (!stillTheSameAddress()) {
+ alternates.remove(0);
+ }
+ return alternates.isEmpty() ? null : alternates.get(0);
Review Comment:
Thanks, if you modify the SPI anyways maybe you can also consider my comment from https://issues.apache.org/jira/browse/CXF-8288. Would be nice to always pass the message/exchange before selecting an alternate.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] coheigea commented on pull request #954: CXF-8712 One-off issue with retries
Posted by GitBox <gi...@apache.org>.
coheigea commented on PR #954:
URL: https://github.com/apache/cxf/pull/954#issuecomment-1260606560
@reta What's the status of this PR?
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org