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

Add jersey-client as dependency of pulsar-client-auth-sasl #10055

Merged
merged 1 commit into from
Mar 28, 2021

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Mar 27, 2021

Motivation

When using AuthenticationSasl, ClassNotFoundException will be thrown:

Exception in thread "main" java.lang.RuntimeException: java.lang.ClassNotFoundException: org.glassfish.jersey.client.JerseyClientBuilder

The jersey-client dependency must be imported. However the version of jersey-client must be consistent with what pulsar-client-auth-sasl depends, which is 2.31 currently. For example, if a newer version of jersey-client was imported like

    <dependency>
      <groupId>org.glassfish.jersey.core</groupId>
      <artifactId>jersey-client</artifactId>
      <version>3.0.1</version>
    </dependency>

Another exception would be thrown:

Exception in thread "main" java.lang.NoClassDefFoundError: jakarta/ws/rs/client/ClientBuilder

So the best way to fix it is adding jersey-client as dependency of pulsar-client-auth-sasl.

Modifications

Add jersey-client as dependency of pulsar-client-auth-sasl.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

@eolivelli
Copy link
Contributor

Why are we able to build that module without adding the dependency?
Isn't it already imported implicitly?

@BewareMyPower
Copy link
Contributor Author

@eolivelli

Here's the completed exception stack.

Exception in thread "main" java.lang.RuntimeException: java.lang.ClassNotFoundException: org.glassfish.jersey.client.JerseyClientBuilder
	at javax.ws.rs.client.ClientBuilder.newBuilder(ClientBuilder.java:110)
	at javax.ws.rs.client.ClientBuilder.newClient(ClientBuilder.java:121)
	at org.apache.pulsar.client.impl.auth.AuthenticationSasl.start(AuthenticationSasl.java:164)
	at org.apache.pulsar.client.impl.PulsarClientImpl.<init>(PulsarClientImpl.java:146)
	at org.apache.pulsar.client.impl.PulsarClientImpl.<init>(PulsarClientImpl.java:134)
	at org.apache.pulsar.client.impl.PulsarClientImpl.<init>(PulsarClientImpl.java:130)
	at org.apache.pulsar.client.impl.ClientBuilderImpl.build(ClientBuilderImpl.java:65)
	at pulsar.KerberosAuthClient.main(KerberosAuthClient.java:27)

From javax.ws.rs.client.ClientBuilder.newBuilder we can see

    public static ClientBuilder newBuilder() {
        try {
            Object delegate = FactoryFinder.find("javax.ws.rs.client.ClientBuilder", "org.glassfish.jersey.client.JerseyClientBuilder", ClientBuilder.class);
            if (!(delegate instanceof ClientBuilder)) {
                Class pClass = ClientBuilder.class;
                String classnameAsResource = pClass.getName().replace('.', '/') + ".class";
                ClassLoader loader = pClass.getClassLoader();
                if (loader == null) {
                    loader = ClassLoader.getSystemClassLoader();
                }

It tries to load JerseyClientBuilder dynamically.

@eolivelli
Copy link
Contributor

Do we have test case? Why the tests are not failing?

@BewareMyPower
Copy link
Contributor Author

Maybe it's because the tests uses the jersey-client dependency from the root pom.xml.

pulsar/pom.xml

Lines 604 to 608 in 1b8da92

<dependency>
<groupId>org.glassfish.jersey.core</groupId>
<artifactId>jersey-client</artifactId>
<version>${jersey.version}</version>
</dependency>

I'm not very familiar to Maven and not sure if it could be loaded by sub-module implicitly in tests.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

Btw it is better to have the explicit dependency here.

I was just asking those questions in order to understand better the problem

@BewareMyPower
Copy link
Contributor Author

/pulsarbot run-failure-checks

@merlimat merlimat added this to the 2.8.0 milestone Mar 28, 2021
@merlimat merlimat merged commit 454b1a5 into apache:master Mar 28, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/auth-sasl-dep branch March 28, 2021 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants