-
Notifications
You must be signed in to change notification settings - Fork 509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update javax dependencies in the client to jakarta #235
Update javax dependencies in the client to jakarta #235
Conversation
Hi @gr4cza we are updating the client and refactoring to make it a bit more modular:
This is currently in works and we plan to release sometime in the next week or so. |
4664a35
to
0e14316
Compare
Hi @v1r3n! |
485ae2a
to
bbfd79d
Compare
Hi @v1r3n! |
Hi @gr4cza yes the refactored version should be in the repo very soon (in 1-2 weeks max), so I would just wait for that. |
Replaced `javax.ws.rs` with `jakarta.ws.rs` and updated related imports and client request handling logic. Modified several classes to use Jakarta libraries instead of deprecated javax libraries and updated dependencies to the latest versions.
bbfd79d
to
1815e13
Compare
Hi @v1r3n ! We are currently undergoing a major Spring Boot 2.7 -> 3.x update in our services, and this client is the last dependency before we can proceed with the upgrade. Thanks a lot! |
Hi @gr4cza,
There will be some breaking changes, though we aim to keep them to a minimum. The Worker API and Client interfaces ( I'll give you a couple of concrete examples of where you could find breaking changes. (1) Jersey Config
public WorkflowClient(ClientConfig config, ClientHandler handler) {
this(config, new DefaultConductorClientConfiguration(), handler);
} (2) Eureka ClientIn the Worker API we've removed the Eureka Client configuration option (from * @param eurekaClient Eureka client - used to identify if the server is in discovery or
* not. When the server goes out of discovery, the polling is terminated. If passed
* null, discovery check is not done.
* @return Builder instance
*/
public Builder withEurekaClient(EurekaClient eurekaClient) {
this.eurekaClient = eurekaClient;
return this;
} Why?These changes are largely driven by "dependency optimization" and a redesign of the client to decouple it from its dependencies. This redesign introduces filters, events and listeners (Extensibility through Callbacks or Event-Driven Architecture, IoC). Take a look at these gists which are the output of You will notice client V3 has a reduced dependency set. The aim is to minimize Classpath pollution and prevent potential conflicts. Take Netflix Eureka as an example, we've encountered version conflicts for users relying on Spring Cloud. Some of them weren't event using Eureka. So, we've decided to remove the direct dependency. In the client it's used by the if (eurekaClient != null
&& !eurekaClient.getInstanceRemoteStatus().equals(InstanceStatus.UP)
&& !discoveryOverride) {
LOGGER.debug("Instance is NOT UP in discovery - will not poll");
return;
} You will be able to achieve the same with a var runnerConfigurer = new TaskRunnerConfigurer
.Builder(taskClient, List.of(new ApprovalWorker()))
.withThreadCount(10)
.withPollFilter((String taskType, String domain) -> {
return eurekaClient.getInstanceRemoteStatus().equals(InstanceStatus.UP);
})
.withListener(PollCompleted.class, (e) -> {
log.info("Poll Completed {}", e);
var timer = prometheusRegistry.timer("poll_success", "type", e.getTaskType());
timer.record(e.getDuration());
})
.withListener(TaskExecutionFailure.class, (e) -> {
log.error("Task Execution Failure {}", e);
var counter = prometheusRegistry.counter("execute_failure", "type", e.getTaskType(), "id", e.getTaskId());
counter.increment();
})
.build();
runnerConfigurer.init(); BTW, the telemetry part was also removed but you can achieve the same with Events and Listeners as shown in the example. Last but not least, if you are using Spring, we are going to provide a module for auto configuration (similar to the one that already exists) |
@gr4cza Sorry, forgot to reply to this. Let me take a closer look at these changes. I think we could get them merged. |
@jmigueprieto Thank you for the explanation! Looking forward to the new OkHttp client as well!
That would be really nice. Based on your description, it would be more beneficial for us to make the migrations in two steps, so we would like to get this MR merged first. Thank you for your help! |
Hi @v1r3n @jmigueprieto! Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. However, there is one warning
org.glassfish.jersey.client.innate.inject.NonInjectionManager <init>
WARNING: Falling back to injection-less client.
which might disturb users a bit but I'm assuming we are deliberately leaving hk2
dependency out.
SEE: https://stackoverflow.com/questions/78403427/jersey-falling-back-to-injection-less-client
For some scenarios, it can be a requirement not to depend on HK2 (when it is not needed) to minimize the application/microservice footprint.
Some people complain about the time the Jersey client takes to boot up before the first request can be sent. Without HK2, the time is about halved.
cc: @v1r3n
Replaced
javax.ws.rs
withjakarta.ws.rs
and updated related imports and client request handling logic. Modified several classes to use Jakarta libraries instead of deprecated javax libraries and updated dependencies to the latest versions.Pull Request type
NOTE: Please remember to run
./gradlew spotlessApply
to fix any format violations.Changes in this PR
Since the core modules have been updated to Spring Boot 3, the client also needed to be updated to use Jakarta dependencies instead of javax, to ensure compatibility with the newest version.
Alternatives considered
Describe alternative implementation you have considered