Skip to content
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

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

gr4cza
Copy link
Contributor

@gr4cza gr4cza commented Aug 8, 2024

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

  • Bugfix
  • Feature
  • 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

@v1r3n
Copy link
Collaborator

v1r3n commented Aug 9, 2024

Hi @gr4cza we are updating the client and refactoring to make it a bit more modular:

  1. Adding OKHttp as a default HTTP client
  2. Updating versions to fix the vulnerabilities
  3. Removing the server side dependencies
  4. 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

@gr4cza gr4cza force-pushed the feat/update_to_jakarta_in_client branch from 4664a35 to 0e14316 Compare August 9, 2024 10:06
@gr4cza
Copy link
Contributor Author

gr4cza commented Aug 9, 2024

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!

@gr4cza gr4cza force-pushed the feat/update_to_jakarta_in_client branch from 485ae2a to bbfd79d Compare August 21, 2024 09:38
@gr4cza
Copy link
Contributor Author

gr4cza commented Aug 22, 2024

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.

@v1r3n
Copy link
Collaborator

v1r3n commented Aug 23, 2024

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.
@gr4cza gr4cza force-pushed the feat/update_to_jakarta_in_client branch from bbfd79d to 1815e13 Compare August 23, 2024 07:29
@gr4cza
Copy link
Contributor Author

gr4cza commented Aug 23, 2024

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!

@jmigueprieto
Copy link
Contributor

jmigueprieto commented Aug 26, 2024

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)

@jmigueprieto
Copy link
Contributor

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.

@gr4cza
Copy link
Contributor Author

gr4cza commented Aug 27, 2024

@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!

@gr4cza
Copy link
Contributor Author

gr4cza commented Sep 2, 2024

Hi @v1r3n @jmigueprieto!
Is there any progress with the review and/or with the refactor?

Thank you!

Copy link
Contributor

@jmigueprieto jmigueprieto left a 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

@v1r3n v1r3n merged commit 6f988f1 into conductor-oss:main Sep 4, 2024
2 checks passed
@gr4cza gr4cza deleted the feat/update_to_jakarta_in_client branch September 4, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants