Add quarkus.cxf.client."client-name".max-same-uri configuration option #1669
Conversation
Will do! Thanks for picking this up and sorry for the delay. I had a high fever the past two days so everything paused - but back on now. When do you plan to have this merged? |
| /* The first element in the retransmits list is the original URI that we do not count as a retransmit */ | ||
| return retransmits.size() - 1; | ||
| } | ||
|
|
||
| void redirectRetransmit(URI newURL) throws IOException { | ||
| if (Log.isDebugEnabled()) { |
There was a problem hiding this comment.
optional: if log.debugf is used for loging the retransmits, is this if statement still needed?
There was a problem hiding this comment.
The if statement could indeed be omitted, if the log statement params would all be some simple values. But in this case, there is redirects.stream().map(URI::toString).collect(Collectors.joining(" -> ")) which costs some CPU cycles, but is useless in case the debug level is not active.
There was a problem hiding this comment.
I never thought about it from a performance POV, but that makes a lot of sense, thanks!
| return Response.status(302).header("Location", "/RedirectRest/selfRedirect").build(); | ||
|
|
||
| if (selfRedirectsCount == resetCount && count == resetCount) { | ||
| redirectCounter.set(0); |
There was a problem hiding this comment.
This is a smart way to do the counting, but one remaining issue is it doesn't really reset the counter to 0 after a test for too many same URIs. You can see that with the current setup with async/maxSameUri2, the counter begins at 1 instead of 0 due to the previous sync test. I don't have a good solution for this (other than to add yet another path param that copies the max same uri property), but would perhaps set the counter to -1 instead of 0 here to at least avoid this problem in this setup in case future tests are added.
There was a problem hiding this comment.
Oh, really, thanks for finding out. We could introduce a separate rest endpoint to reset the counter.
There was a problem hiding this comment.
Do you mean a separate endpoint only for the too many same uri test, which basically doesn't keep a counter and just keeps redirecting to self? So this endpoint is only kept for the tests with max-uri > number of self redirects, which means it always resets to 0 after?
There was a problem hiding this comment.
I looked at it again after a night's sleep. I think a new endpoint is not needed. If you change the URI of the maxSameURi2 test to selfRedirect/3/3 it should solve the issue, which makes sense because the selfRedirectCount indicates how many self redirects there should be before the test completes (either going to a real endpoint or throwing an error), and for maxSameUri2 we need 3 self redirects before an error is thrown.
The condition can also be simplified to
if (count == resetCount) { redirectCounter.set(0); }
There was a problem hiding this comment.
See 7bb1c1b where I added RedirectRest.selfRedirect() for resetting the counter.
There was a problem hiding this comment.
I never thought about it that way, interesting solution. Question: why do the try-finally in the redirect test instead of splitting it into two tests (small/large body) and let the BeforeEach method do the job?
There was a problem hiding this comment.
No specific reason, could be done that way too.
| + " but already performed maximum number 1 of allowed retransmits;" | ||
| + " you may want to increase quarkus.cxf.client.doubleRedirectMaxRetransmits1.max-retransmits." | ||
| + " Visited URIs: http://localhost:\\E[0-9]+\\Q/RedirectRest/doubleRedirect -> http://localhost:\\E[0-9]+\\Q/RedirectRest/singleRedirect\\E")) | ||
| .or(CoreMatchers.containsString("Unexpected EOF in prolog"))); |
There was a problem hiding this comment.
question: why is the second matcher necessary? The test seems to only return the first string. Same for the other tests with two matchers.
There was a problem hiding this comment.
The same tests are run with QUARKUS_CXF_DEFAULT_HTTP_CONDUIT_FACTORY=URLConnectionHTTPConduitFactory environment variable https://github.com/quarkiverse/quarkus-cxf/blob/main/.github/actions/build-and-run-jvm-tests/action.yml#L44-L49
The alternative set of error messages is for the URLConnectionHTTPConduit.
There was a problem hiding this comment.
Didn't realize that, thank you!
Fix #1639
@dcheng1248 I dared to pick your work from dcheng1248#1 and adapted to make it work the way I think is appropriate. Please review, if you like.