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

KAFKA-6562: (follow-up) Publish "jackson-databind" lib as provided scope dependency in clients maven artifact #5110

Merged
merged 3 commits into from
Jun 12, 2018

Conversation

omkreddy
Copy link
Contributor

@omkreddy omkreddy commented May 31, 2018

  1. jackson-databind library is mapped as a provided library by using maven plugin configuration-to-scope mapping feature.
    https://docs.gradle.org/current/javadoc/org/gradle/api/artifacts/maven/Conf2ScopeMappingContainer.html
  2. Clients using default implementation of SASL/OAUTHBEARER mechanism must provide jackson-databind library to Kafka client runtime.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@omkreddy omkreddy force-pushed the KAFKA-6562-FOLLOWUP-1 branch 2 times, most recently from 997e806 to f4ce46e Compare May 31, 2018 19:38
@omkreddy omkreddy changed the title KAFKA-6562: (follow-up) Publish "jackson-databind" lib as provided scope dependency of kafka clients library KAFKA-6562: (follow-up) Publish "jackson-databind" lib as provided scope dependency in clients maven artifact May 31, 2018
@omkreddy
Copy link
Contributor Author

@ijuma @rajinisivaram Please take a look when you get a chance.

sample generated pom file:
kafka-clients-2.0.0-SNAPSHOT.pom.txt

compile libs.jacksonDatabind // for SASL/OAUTHBEARER bearer token parsing
compileOnly libs.jacksonDatabind // for SASL/OAUTHBEARER bearer token parsing

jacksonDatabindConfig libs.jacksonDatabind // to publish as provided scope dependency.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we don't include this?

Copy link
Contributor Author

@omkreddy omkreddy Jun 1, 2018

Choose a reason for hiding this comment

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

We will not get below provided scope dependency in clients maven pom file. Like regular maven based projects, it is good to publish provided scope libraries. Otherwise users will not get to know all the required libs (users may not always read the docs).

   <dependency>
      <groupId>com.fasterxml.jackson.core</groupId>
      <artifactId>jackson-databind</artifactId>
      <version>2.9.5</version>
      <scope>provided</scope>
    </dependency>

@ijuma ijuma added this to the 2.0.0 milestone Jun 1, 2018
@omkreddy
Copy link
Contributor Author

omkreddy commented Jun 1, 2018

retest this please

build.gradle Outdated
jacksonDatabindConfig
}

conf2ScopeMappings.addMapping(1000, configurations.jacksonDatabindConfig, "provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

What does 1000 mean here? Probably worth adding a comment.

Copy link
Contributor Author

@omkreddy omkreddy Jun 6, 2018

Choose a reason for hiding this comment

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

Its a priority number used for comparison with the priority of other scopes. If multiple configurations are mapped with different scopes, then high priority scope will be select ed.

It on our case we have only one configuration, So its not a issue. I just gave 1000 (highest priority) for the provided scope.

Added a comment.

Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM apart from one minor comment. @rajinisivaram, does this look good to you too?

Copy link
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

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

@ijuma Yes, looks good. @omkreddy Thanks for the PR.

@omkreddy
Copy link
Contributor Author

omkreddy commented Jun 8, 2018

@ijuma Can we merge this?

@@ -712,6 +712,9 @@ <h3><a id="security_sasl" href="#security_sasl">7.3 Authentication using SASL</a
<pre>
security.protocol=SASL_SSL (or SASL_PLAINTEXT if non-production)
sasl.mechanism=OAUTHBEARER</pre></li>
<li>The default implementation of SASL/OAUTHBEARER depends on com.fasterxml.jackson.core:jackson-databind library.
This is added as provided scope dependency in Kafka clients maven artifact. Users must provide jackson-databind library
to Kafka client runtime.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't notice this the first time, but this wording is a bit unclear. Maybe something like:

"The default implementation of SASL/OAUTHBEARER depends on the jackson-databind library. Since it's an optional dependency, users have to configure it as a dependency via their build tool. The maven coordinates are..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated docs. Since correct version will be available in pom file and it can change from release to release, I have not included maven links. Pls take a look.

Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM

@ijuma ijuma merged commit dbca6b9 into apache:trunk Jun 12, 2018
ijuma pushed a commit that referenced this pull request Jun 12, 2018
Use `provided` scope in Maven.

Reviewers: Rajini Sivaram <[email protected]>, Ismael Juma <[email protected]>
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
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.

3 participants