KAFKA-12415: Prepare for Gradle 7.0 and restrict transitive scope for non api dependencies by ijuma · Pull Request #10203 · apache/kafka

ijuma · 2021-02-24T16:37:31Z

Gradle 7.0 is required for Java 16 compatibility and it removes a number of
deprecated APIs. Fix most issues preventing the upgrade to Gradle 7.0.
The remaining ones are more complicated and should be handled
in a separate PR. Details of the changes:

Release tarball no longer includes includes test, sources, javadoc and test sources jars (these
are still published to the Maven Central repository).
Replace compile with api or implementation - note that implementation
dependencies appear with runtime scope in the pom file so this is a (positive)
change in behavior
Add missing dependencies that were uncovered by the usage of implementation
Replace testCompile with testImplementation
Replace runtime with runtimeOnly and testRuntime with testRuntimeOnly
Replace configurations.runtime with configurations.runtimeClasspath
Replace configurations.testRuntime with configurations.testRuntimeClasspath (except for
the usage in the streams project as that causes a cyclic dependency error)
Use java-library plugin instead of java
Use maven-publish plugin instead of deprecated maven plugin - this changes the
commands used to publish and to install locally, but task aliases for install and
uploadArchives were added for backwards compatibility
Removed -x signArchives line from the readme since it was wrong (it was a
no-op before and it fails now, however)
Replaces artifacts block with an approach that works with the maven-publish plugin
Don't publish jmh-benchmark module - the shadow jar is pretty large and not
particularly useful (before this PR, we would publish the non shadow jars)
Replace version with archiveVersion, baseName with archiveBaseName and
classifier with archiveClassifier
Update Gradle and plugins to the latest stable version (7.0 is not stable yet)
Use plugin DSL to configure plugins
Updated notable changes for 3.0

Committer Checklist (excluded from commit message)

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

…uration

ijuma · 2021-02-24T16:40:22Z

We should also change configurations.testRuntime a few lines above to configurations.testRuntimeClasspath, but this causes a cyclic build error. Fixing this will be required before we move to Gradle 7.0.

Is there a jira already?

https://issues.apache.org/jira/browse/KAFKA-12417

ijuma · 2021-02-25T09:25:05Z


-apply plugin: "com.diffplug.spotless"
+plugins {
+  id 'com.diffplug.spotless' version '5.10.2'


We have to either include the version literal here or in a properties file. Methods cannot be invoked. Having a separate properties file and the dependencies file didn't seem particularly better than this way.

ijuma · 2021-02-25T09:27:34Z

@chia7712 @omkreddy Any of you have cycles to review this? We reached the end of the road when it comes to delaying the handling of these Gradle deprecations. :)

chia7712

@ijuma thanks for this nice patch. Left some comments for this PR. please take a look.

…es to the publishing task anyway

chia7712

@ijuma Thanks for updating code. please take a look at following minor questions.

ijuma · 2021-02-27T17:12:24Z

@mjsax @guozhangwang @vvcephei Can one of you review the build changes for the stream modules and check if they all make sense?

ijuma · 2021-03-01T16:27:58Z

@chia7712 Are you ok after the latest changes?

chia7712

@ijuma thanks for updating patch. a couple of comments are left.

chia7712 · 2021-03-02T04:53:01Z

Is there a jira already?

* apache-github/trunk: (37 commits) KAFKA-10357: Extract setup of changelog from Streams partition assignor (apache#10163) KAFKA-10251: increase timeout for consuming records (apache#10228) KAFKA-12394; Return `TOPIC_AUTHORIZATION_FAILED` in delete topic response if no describe permission (apache#10223) MINOR: Disable transactional/idempotent system tests for Raft quorums (apache#10224) KAFKA-10766: Unit test cases for RocksDBRangeIterator (apache#9717) KAFKA-12289: Adding test cases for prefix scan in InMemoryKeyValueStore (apache#10052) KAFKA-12268: Implement task idling semantics via currentLag API (apache#10137) MINOR: Time and log producer state recovery phases (apache#10241) MINOR: correct the error message of validating uint32 (apache#10193) MINOR: Format the revoking active log output in `StreamsPartitionAssignor` (apache#10242) KAFKA-12323 Follow-up: Refactor the unit test a bit (apache#10205) MINOR: Remove stack trace of the lock exception in a debug log4j (apache#10231) MINOR: Word count should account for extra whitespaces between words (apache#10229) MINOR; Small refactor in `GroupMetadata` (apache#10236) KAFKA-10340: Proactively close producer when cancelling source tasks (apache#10016) KAFKA-12329; kafka-reassign-partitions command should give a better error message when a topic does not exist (apache#10141) KAFKA-12254: Ensure MM2 creates topics with source topic configs (apache#10217) MINOR: fix kafka-metadata-shell.sh (apache#10226) KAFKA-12374: Add missing config sasl.mechanism.controller.protocol (apache#10199) KAFKA-10101: Fix edge cases in Log.recoverLog and LogManager.loadLogs (apache#8812) ...

rhauch · 2021-03-03T08:14:18Z

@ijuma, I ran ./gradlew clean install releaseTarGz -x signArchives (which is still in the README) and it failed with:

* What went wrong:
Task 'signArchives' not found in root project 'kafka'.

If that should not work, we should also update the README. But if it should work, we may need a bit more work for building an unsigned release archive.

FWIW, running ./gradew clean install releaseTarGz and ./gradlewAll publish both appear to work as expected.

rhauch · 2021-03-03T08:33:52Z

A few more things regarding ./gradlewAll publishToMavenLocal or ./gradlewAll install:

These do not appear to publish source, test or test source JARs to the local Maven repository, unlike ./gradlewAll install on the 2.8 branch.
Is it intentional to no longer publish .asc files to the local Maven repository? I do see with this PR the install commands now publish .module files that contain the various hashes, but I don't know if that's expected.

I have Gradle 6.8.3 installed.

* apache-github/trunk: KAFKA-12400: Upgrade jetty to fix CVE-2020-27223 MINOR: Fix null exception in coordinator log (apache#10250) y KAFKA-12375: don't reuse thread.id until a thread has fully shut down (apache#10215) KAFKA-12177: apply log start offset retention before time and size based retention (apache#10216) KAFKA-12369; Implement `ListTransactions` API (apache#10206)

…g that does

ijuma · 2021-03-03T15:28:37Z

Thanks for testing @rhauch!

@ijuma, I ran ./gradlew clean install releaseTarGz -x signArchives (which is still in the README) and it failed with:

The readme was wrong when it comes to this since signing was only triggered when uploadArchives was called. I updated the readme instead of making code changes.

These do not appear to publish source, test or test source JARs to the local Maven repository, unlike ./gradlewAll install on the 2.8 branch.

Good catch, I updated the build so that this works with the new plugin.

Is it intentional to no longer publish .asc files to the local Maven repository? I do see with this PR the install commands now publish .module files that contain the various hashes, but I don't know if that's expected.

It seems like the new plugin only includes those when publish is called instead of publishToMavenLocal. That seems fine, I don't see any reason why those files are useful when installing to the local repo.

chia7712 · 2021-03-03T15:57:34Z

The libs in distribution get changed.

before

after

Not sure whether it breaks something (for example, users want to run main class from test code?)

chia7712 · 2021-03-03T16:56:21Z


  shadowJar {
-    baseName = 'kafka-jmh-benchmarks-all'
-    classifier = null


Those changes rename the shadow jar from kafka-jmh-benchmarks-all.jar to kafka-jmh-benchmarks-all-3.0.0-SNAPSHOT-all.jar. That breaks jmh.sh as the script presumes the name of shadow jar is kafka-jmh-benchmarks-all.jar (https://github.com/apache/kafka/blob/trunk/jmh-benchmarks/jmh.sh#L40)

Good point, fixed the script and readme.

Could we remove duplicate "all" from name of shadow jar? kafka-jmh-benchmarks-all-3.0.0-SNAPSHOT-all.jar

rhauch

The Connect portion looks correct. Thanks, @ijuma!

rhauch · 2021-03-03T17:18:55Z

-    compile libs.jacksonDatabind
-    compile libs.jacksonJDK8Datatypes
-    compile libs.slf4jApi
+    implementation project(':connect:api')


I think this is the correct mapping.

By switching to implementation, any external project depending on the connect:json module will have to include an explicit dependency on connect:api. This appears to be have the same effect as what previous branches do with compile on earlier branches, but using implementation seems more correct since the Gradle docs suggest that compile (or compileOnly) are more applicable for shading, which is not our case here.

Does connect-json expose any connect-api types in its public API?

The public API is defined in connect:api, and connect:json and connect:transforms are implementations of interfaces defined in (and therefore cannot be used without) connect:api. Within AK, we provide the connect:json and connect:transforms always with the connect:api (and runtime) modules, and therefore using implementation (and compile in earlier branches) seems appropriate.

From a more abstract perspective, one might argue that the connect:json and connect:transforms could declare their use of connect:api as a transitive dependency (e.g., api rather than implementation). It's just that as a practical matter we don't need this, and arguably no other projects would either.

As such, I think it's safer to use implementation, as it's in line with what we were doing earlier.

To clarify, compile is more like api than implementation. I switched the dependency of connect-api for connect-json and connect-transform to api for now. We can change this in a future PR, if we like, but it seems more consistent with the generally conservative approach we're taking in this PR.

chia7712

LGTM

ijuma · 2021-03-04T13:57:28Z

@chia7712 I filed https://issues.apache.org/jira/browse/KAFKA-12418 to follow up on the jars that are no longer included in the release tarball. It seems OK to me, but I'll do a bit more due dilligence before the 3.0.0 release.

ijuma · 2021-03-04T19:21:05Z

No test failures, but one of the jobs was killed in the last run:
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-10203/19/testReport/

The previous runs confirm that it's a flaky issue unrelated to this PR (there are no test changes here).

ableegoldman · 2021-03-11T23:51:13Z


-The above command will fail if you haven't set up the signing key. To bypass signing the artifact, you can run:
-
-    ./gradlew clean releaseTarGz -x signArchives


@ijuma the proper way to skip signing is now to run ./gradlew clean releaseTarGz -DskipSigning, is that right? Is there a reason we removed this instruction from the README instead of updating it?

Signing is skipped by default. Are you seeing something different? There is a table that describes skipSigning:

https://github.com/apache/kafka#common-build-options

Yes, we're suddenly seeing our soak test (an application which deploys from trunk) fail with

./gradlew clean install releaseTarGz > Configure project : Building project 'core' with Scala version 2.13.5 Building project 'streams-scala' with Scala version 2.13.5 > Task :connect:signMavenJavaPublication FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':connect:signMavenJavaPublication'. > Cannot perform signing task ':connect:signMavenJavaPublication' because it has no configured signatory

I personally haven't been able to reproduce this, but both @wcarlson5 and @lct45 have reported running into this issue

Note that your command has install while the README line I deleted did not (and was wrong). It is still weird that you're seeing this for snapshot builds. I'll work with you all offline to try and figure it out.

We figured it out, the build was using a non snapshot version. Passing -PskipSigning=true fixed it. Submitted #10307 to make it easier to debug these issues in the future.

ijuma added 4 commits February 24, 2021 07:18

Replace maven with maven-publish and use plugin dsl for plugin config…

784cc81

…uration

Comment out and remove maven plugin code

65a1ad0

compile, testCompile, runtime, testRuntime were all removed

f51d0ea

Fix cyclic issue affecting Streams

59f3756

ijuma commented Feb 24, 2021

View reviewed changes

Use java-library and use api/implementation correctly

78b40f2

ijuma force-pushed the gradle-7.0-ready branch from dac7e6b to 78b40f2 Compare February 25, 2021 03:58

ijuma added 4 commits February 24, 2021 20:02

Fix more deprecations

d64ef6e

Configure maven publisher

5507285

Update gradle and plugins

cd36077

Don't publish jmh-benchmark module

d54a8be

ijuma marked this pull request as ready for review February 25, 2021 09:20

ijuma commented Feb 25, 2021

View reviewed changes

chia7712 reviewed Feb 26, 2021

View reviewed changes

Comment thread build.gradle Outdated

Comment thread build.gradle

Comment thread README.md

Comment thread build.gradle Outdated

ijuma added 6 commits February 26, 2021 06:30

Use publishToMavenLocal in Jenkinsfile

f73e84b

Add task aliases for backward compatibility

56d5a9b

shouldSign doesn't need to check the task names since it only appli…

cabdc94

…es to the publishing task anyway

Improve comment

9b6b730

Use api for some of connect-json project dependencies

5498ad9

More tweaks

c084401

chia7712 reviewed Feb 27, 2021

View reviewed changes

Comment thread build.gradle

Comment thread build.gradle Outdated

streams connect-json dependency should be api

7064e91

chia7712 reviewed Mar 2, 2021

View reviewed changes

ijuma added 3 commits March 2, 2021 12:36

Update readme to mention publish as the recommended command

7c87c6a

Export streams and clients apis from streams-test-utils

73c9b0f

ijuma added 4 commits March 3, 2021 05:28

Remove inaccurate line about key signing

83b2411

artifacts doesn't work with maven publish, replace it with somethin…

1f257e5

…g that does

classifier -> archiveClassifier

b3205f5

chia7712 reviewed Mar 3, 2021

View reviewed changes

Comment thread build.gradle Outdated

Fix folder name in release tar gz

b365ee9

chia7712 reviewed Mar 3, 2021

View reviewed changes

rhauch approved these changes Mar 3, 2021

View reviewed changes

mjsax mentioned this pull request Mar 3, 2021

KAFKA-5146 / move kafka-streams example to a new module #10131

Merged

3 tasks

Fix jmh-benchmark script since the jar now includes a version

7f0281b

chia7712 approved these changes Mar 4, 2021

View reviewed changes

ijuma added 2 commits March 4, 2021 05:27

Remove duplicate all from jmh benchmarks name

13fa697

Use api for connect-api in connect-json and connect-transforms.

e581e56

chia7712 approved these changes Mar 4, 2021

View reviewed changes

ijuma changed the title ~~MINOR: Prepare for Gradle 7.0~~ KAFKA-12415 Prepare for Gradle 7.0 and restrict transitive scope for non api dependencies Mar 4, 2021

Add notable changes as a result of this PR

9c35471

ijuma merged commit 7a3ebbe into apache:trunk Mar 4, 2021

ijuma deleted the gradle-7.0-ready branch March 4, 2021 19:22

ijuma changed the title ~~KAFKA-12415 Prepare for Gradle 7.0 and restrict transitive scope for non api dependencies~~ KAFKA-12415: Prepare for Gradle 7.0 and restrict transitive scope for non api dependencies Mar 4, 2021

ableegoldman reviewed Mar 11, 2021

View reviewed changes

ableegoldman mentioned this pull request Mar 17, 2021

chore: add rocksdbjni as explicit dependency to config setter module confluentinc/ksql#7262

Merged

2 tasks

dejan2609 mentioned this pull request Apr 28, 2021

KAFKA-12728: version upgrades: gradle (6.8.3 -->> 7.0.2) and gradle shadow plugin (6.1.0 -->> 7.0.0) #10606

Merged

soarez mentioned this pull request May 18, 2021

MINOR: Remove use of deprecated Gradle syntax #10195

Closed

3 tasks

garyrussell mentioned this pull request Sep 29, 2021

GH-1966: Upgrade Kafka Clients to 3.0.0 spring-projects/spring-kafka#1967

Merged

ocadaruma mentioned this pull request May 27, 2022

Upgrade kafka-clients dependency to 3.2.0 line/decaton#154

Merged


		The above command will fail if you haven't set up the signing key. To bypass signing the artifact, you can run:

		./gradlew clean releaseTarGz -x signArchives

Conversation

ijuma commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Feb 25, 2021

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ijuma commented Feb 27, 2021

Uh oh!

ijuma commented Mar 1, 2021

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhauch commented Mar 3, 2021

Uh oh!

rhauch commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Mar 3, 2021

Uh oh!

Uh oh!

chia7712 commented Mar 3, 2021

before

after

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Mar 4, 2021

ijuma commented Feb 24, 2021 •

edited

Loading

rhauch commented Mar 3, 2021 •

edited

Loading

ijuma Mar 3, 2021 •

edited

Loading

ijuma Mar 4, 2021 •

edited

Loading

ijuma commented Mar 4, 2021 •

edited

Loading

ableegoldman Mar 12, 2021 •

edited

Loading