Migration to Spring Boot 3 #1471
Conversation
HTTP Client 5 has a DefaultHttpRequestRetryStrategy, which by default retries 1 time after receiving a 503 status. The mock servers were changed to return a 500 status, so that this behavior does not kick in.
- HTTP Client 5 has a DefaultHttpRequestRetryStrategy, which by default retries 1 time after receiving a 503 status. The mock servers were changed to return a 500 status, so that this behavior does not kick in. - Use setSocketTimeout instead of setConnectTimeout
Use setSocketTimeout instead of setConnectTimeout
Use setSocketTimeout instead of setConnectTimeout
Use newer version of jaxws-rt dependency
On Windows operating systems, some tests failed as they required hostname to be "localhost", but Windows returned "127.0.0.1".
HttpClient#execute(ClassicHttpRequest) is deprecated. Using executeOpen() shows the intent that the response should be kept open.
msdousti
requested review from
whiskeysierra,
lukasniemeier-zalando,
fatroom,
Semernitskaya,
dingxiangfei2009,
Joyfolk and
gpradeepkrishna
as code owners
maven-build-cache-extension improves build times. It requires Maven 3.9.
...lient/src/test/java/org/zalando/riptide/httpclient/BufferingApacheClientHttpRequestTest.java
Outdated
Show resolved
Hide resolved
...ncy/src/test/java/org/zalando/riptide/idempotency/MethodOverrideIdempotencyDetectorTest.java
Outdated
Show resolved
Hide resolved
|
I tried very hard in my first attempt to avoid the formatting changes as much as possible, which sometimes meant to painfully re-order re-ordered imports done by IntelliJ. There is now the issue that in between all those changes, there are also formatting changes applied. What is your take on this, @fatroom? |
|
Any reason why you went with the commit 7052bff at all given that the project uses dependabot? |
| @Configuration | ||
| @ConditionalOnClass(HttpClient.class) | ||
| @ConditionalOnMissingBean(FlowHttpRequestInterceptor.class) | ||
| @ConditionalOnProperty(name = "opentracing.flowid.httpclient.enabled", havingValue = "true", matchIfMissing = true) |
There was a problem hiding this comment.
matchIfMissing option should be false here.
But generally I'm concerned that http client tries now to assume that it's working in http servlet environment and establishing classes on incoming layer of the app.
There was a problem hiding this comment.
matchIfMissing option should be false here.
The tests won't pass if it's set to false - and I guess it will break existing Riptide users because they may not have set opentracing.flowid.httpclient.enabled by default.
We can instead enable it with either riptide.defaults.tracing.enabled = true or riptide.clients.*.tracing.enabled = true being true (the latter having a wildcard makes it a bit hard 🤔).
But generally I'm concerned that http client tries now to assume that it's working in http servlet environment and establishing classes on incoming layer of the app.
To my understanding, it doesn't make this assumption. However, if it works in such environment, Spring gets the hint how to order auto-configurations.
The current version of Riptide works in the same way - it just imports this class from opentracing-flowid-starter:
https://github.com/zalando/opentracing-toolbox/blob/main/opentracing-flowid/opentracing-flowid-autoconfigure/src/main/java/org/zalando/opentracing/flowid/autoconfigure/OpenTracingFlowIdAutoConfiguration.java
Since that project is in maintenance mode, I thought the only way to keep OpenTracing is add this little configuration.
There was a problem hiding this comment.
I think, it was always a bit weird that the FlowId topic was linked to Open Tracing. But I think, this was the issue when the original tracer project was started.
While it has something to do with tracing in general, in the end it is just an interceptor fiddling with the X-Flow-Id header. So given that we want to release a major version, we should think about whether it makes sense to decouple it and just use the tracing.propagate-flow-id property for it?
There was a problem hiding this comment.
I agree that it can be an improvement to decouple it, but it falls out the scope of this PR, which is "just" migration to SB 3.😊
We can have another task in the milestone of Release 4.0 to apply the suggested decoupling. The rationale behind not doing so is to give this PR a chance to being released as soon as possible.
With Logbook, we did multiple release candidates, used them in our services, found the issues, and fixed them until we could make the new major release. I can imaginge we'll do the same, and in the meantime we have the chance to work on a decopling.
|
@cberg-zalando I'm fine with using default formatting of IDEA here. So star imports and re-ordering are acceptable. Related to dependency updates - as well not a big problem, some of this updates aren't merged to master yet, or missed by dependabot. The only questionable dependency change - is going to milestone release of junit |
Using the technique mentioned here: https://unix.stackexchange.com/a/161853/500398
I just wanted to have a judgement call. |
|
@cberg-zalando can you update MIGRATION.md with changes from this PR? |
Will do when I have the time available. When I started the migration, I had more time available. But I guess, you know how things can go. |
- Documented behaviour change due to update to Apache HttpClient 5
|
This looks fine for me now. |
|
👍 |
Migration to Spring Boot 3 along with
apache components >= 5.1andJakarta EE >= 9.Motivation and Context
Resolves #1351
Types of changes
Checklist: