conductor-oss
Update javax dependencies in the client to jakarta
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.
Pull Request type
- [x] Bugfix
- [ ] Feature
- [x] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] WHOSUSING.md
- [ ] Other (please describe):
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
Hi @gr4cza we are updating the client and refactoring to make it a bit more modular:
- Adding OKHttp as a default HTTP client
- Updating versions to fix the vulnerabilities
- Removing the server side dependencies
- Adding some automated code generation to better help client stay updated with server
This is currently in works and we plan to release sometime in the next week or so. cc: @jmigueprieto
Hi @v1r3n! It is really great to hear! Looking forward to try it out! If my PR is unnecessary/obsolete feel free to close it!
Hi @v1r3n! Is there any progress on the refactoring? In the mean time, i've tried to fix every broken test on this one. Thanks.
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.
Hi @v1r3n ! How much change will the refactoring cause in the client interface?
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. If the refactor brings significant changes, I would prefer to have this client included first so that we can migrate to OkHttp incrementally later, rather than doing it all at once with the Spring update.
Thanks a lot!
Hi @gr4cza,
How much change will the refactoring cause in the client interface?
There will be some breaking changes, though we aim to keep them to a minimum. The Worker API and Client interfaces (com.netflix.conductor.client.http.*) will remain functionally compatible for the most part.
I'll give you a couple of concrete examples of where you could find breaking changes.
(1) Jersey Config
WorkflowClient and other clients will have all the exact same methods but constructors like this one where there is a dependency on Jersey are going to be removed.
public WorkflowClient(ClientConfig config, ClientHandler handler) {
this(config, new DefaultConductorClientConfiguration(), handler);
}
(2) Eureka Client
In the Worker API we've removed the Eureka Client configuration option (from TaskRunnerConfigurer).
* @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 gradlew -p client dependencies for all configurations except test configurations.
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 TaskPollExecutor before polling to make the following check:
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 PollFilter. It could look something like this:
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)
If the refactor brings significant changes, I would prefer to have this client included first so that we can migrate to OkHttp incrementally later, rather than doing it all at once with the Spring update.
@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!
Let me take a closer look at these changes. I think we could get them merged.
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! Is there any progress with the review and/or with the refactor?
Thank you!