The Wayback Machine - https://web.archive.org/web/20220125175134/https://github.com/apache/kafka/pull/7677
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

MINOR: Update to Gradle 6.3 #7677

Merged
merged 6 commits into from Apr 20, 2020
Merged

MINOR: Update to Gradle 6.3 #7677

merged 6 commits into from Apr 20, 2020

Conversation

@ijuma
Copy link
Contributor

@ijuma ijuma commented Nov 11, 2019

  • Introduce gradlewAll script to replace *All tasks since the approach
    used by the latter doesn't work since Gradle 6.0 and it's unclear when,
    if ever, it will work again ( see gradle/gradle#11301).
  • Update release script and README given the above.
  • Update zinc to 1.3.5.
  • Update gradle-versions-plugin to 0.28.0.

The major improvements in Gradle 6.0 to 6.3 are:

  • Improved incremental compilation for Scala
  • Support for Java 14 (although some Gradle plugins
    like spotBugs may need to be updated or disabled,
    will do that separately)
  • Improved scalac reporting, warnings are clearly
    marked as such, which is very helpful.

Tested gradlewAll manually for the commands listed in the README
and release script. For uploadArchive, I tested it with a local Maven
repository.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)
@ijuma ijuma force-pushed the gradle-6.0 branch 2 times, most recently from 8d35848 to 1d6c9c0 Nov 11, 2019
@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Nov 11, 2019

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Nov 12, 2019

retest this please

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Nov 21, 2019

retest this please

@ijuma ijuma requested review from omkreddy and hachikuji Nov 21, 2019
@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Nov 21, 2019

retest this please

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Nov 22, 2019

retest this please

@ijuma ijuma changed the title MINOR: Update to Gradle 6.0 MINOR: Update to Gradle 6.0.1 Nov 22, 2019
@omkreddy
Copy link
Contributor

@omkreddy omkreddy commented Nov 23, 2019

@ijuma I am getting errors when I try "all" targets. not sure if this is due to gradle 6 or my env issue.

 ./gradlew clean jarAll

> Configure project :
Building project 'core' with Scala version 2.12.10
Building project 'streams-scala' with Scala version 2.12.10

> Task :jarScala_2_12 FAILED

Execution failed for task ':jarScala_2_12'.
> Included build /Users/mkumar/workspace/kafka has build path :kafka which is the same as included build /Users/mkumar/workspace/kafka
@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Nov 24, 2019

@omkreddy Thanks for testing. It looks like that issue happens with trunk too. Is that the case for you too?

@omkreddy
Copy link
Contributor

@omkreddy omkreddy commented Nov 24, 2019

@ijuma It happened once. But after killing Gradle Daemon process, it never happened on trunk (ran multiple times).

some related links:
gradle/gradle#11301
https://docs.gradle.org/current/userguide/upgrading_version_5.html#changes_to_build_and_task_names_in_composite_builds

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Jan 8, 2020

This is blocked due to the Gradle issue @omkreddy mentioned. I included some of the changes in this PR in #7909. Once that's merged, I will rebase this branch and it should be only the Gradle version bump.

ijuma added 3 commits Apr 5, 2020
The major improvements are:
- Improved incremental compilation for Scala
- Support for Java 13 (although some Gradle plugins
like spotBugs need to be updated or disabled and
Scala inlining needs to be disabled, will do
them in a separate change)
- Improved scalac reporting, warnings are clearly
marked as such, which is very helpful.
@ijuma ijuma changed the title MINOR: Update to Gradle 6.0.1 MINOR: Update to Gradle 6.3 Apr 6, 2020
@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 6, 2020

@omkreddy Given that the Gradle issue doesn't seem to be resolved anytime soon, I moved the logic to run a gradle command with multiple Scala versions to a gradlewAll script. What do you think?

Copy link
Contributor

@omkreddy omkreddy left a comment

@ijuma Thanks for the update. LGTM. I assume we have tested the changes locally.

./gradlew releaseTarGzAll
./gradlewAll test
./gradlewAll jar
./gradlewAll releaseTarGz
Copy link
Contributor

@omkreddy omkreddy Apr 6, 2020

Choose a reason for hiding this comment

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

We also need to update ./gradlew uploadArchivesAll, ./gradlew installAll references below

cmd("Copying artifacts", "cp %s/core/build/distributions/* %s" % (kafka_dir, artifacts_dir), shell=True)
cmd("Copying artifacts", "cp -R %s/build/docs/javadoc %s" % (kafka_dir, artifacts_dir))
cmd("Building docs", "./gradlew aggregatedJavadoc", cwd=kafka_dir, env=jdk8_env)
cmd("Copying docs", "cp -R %s/build/docs/javadoc %s" % (kafka_dir, artifacts_dir))

Copy link
Contributor

@omkreddy omkreddy Apr 6, 2020

Choose a reason for hiding this comment

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

need to update ./gradlew uploadArchivesAll at line no: 644

@chia7712
Copy link
Contributor

@chia7712 chia7712 commented Apr 6, 2020

The scala compilation get slower after applying this PR ( 3m50.56s -> 4m49.70s). The build command is ./gradlew clean build -x test. Not sure whether this happens on my local only.

related issue: gradle/gradle#12591

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 6, 2020

@chia7712 That's because you have clean in the command. The benefit of using zinc is that incremental recompilation is much faster. If you try the same test where you just change one or two files and don't run clean, you should see a significant perf improvement.

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 6, 2020

One job passed and the other failed with a flaky test failure:

org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testReplication

@chia7712
Copy link
Contributor

@chia7712 chia7712 commented Apr 6, 2020

That's because you have clean in the command. The benefit of using zinc is that incremental recompilation is much faster. If you try the same test where you just change one or two files and don't run clean, you should see a significant perf improvement.

thanks for the explanation. My comment was not clear. The question was the clean + build get slower after updating gradle to 6.x (unrelated to this PR. the only change is gradle version). According to the gradle profile report, the duration of compiling scala module become higher ( core: 1m38.90s -> 2m11.35s, streams:streams-scala: 4.525s -> 23.316s). By contrast, the duration of compiling java modules are almost same. loop 5 times and it seems the duration is consistently higher on gradle 6.x

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 6, 2020

@chia7712 I understood. :) Incremental compilation tracking has a cost. The new version of Gradle uses a new version of Zinc which trades full compilation speed for incremental compilation speed. I think that's fine given that incremental compilation is what developers care most about.

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 6, 2020

I assume we have tested the changes locally.

I wanted to get some feedback on the approach, so I had just sanity checked it. I will do more thorough verification before merging.

@lure
Copy link

@lure lure commented Apr 8, 2020

@ijuma, would you be so kind to provide a link on that zinc changes if you have it somewhere near you?
Thank you.

Upd: got an answer from Guillaume Martres on https://gitter.im/sbt/zinc-contrib they say it is not how zinc behaves, and most probably is bug in Gradle.

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 8, 2020

@lure In the past, sbt with incremental compiler vs ant has shown similar behavior where the former was faster under incremental compilation while ant was faster under clean builds. The comment in that chat says the build time doubled, but that's not what was reported here (3m50.56s -> 4m49.70s).

@lure
Copy link

@lure lure commented Apr 8, 2020

That's right, my case shows nearly x2, i.e. 6mins become 11.
1 minute is not expected 2% too. I mean, there shouldn't be slowing at all, new Zinc promises speed up instead.

In my case the problem is reproducible just by replacing Gradle version from 5.6.2 to 6.3.

@chia7712 located particular change that introduced regress, I believe?

Will check my project module by module, may be it is something else.

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 8, 2020

@lure This is the issue I was thinking of where things regressed previously with better incremental recompilation:

gradle/gradle#1475 (comment)

It does look like this time, it may be a Gradle regression in 6.0 as per the issue @chia7712 linked to.

@lure
Copy link

@lure lure commented Apr 8, 2020

It would be great if this is just a regress. :) The issue I observe is reproducible on 6.2 too so it could be the case.
We do not use ant/parallel options and it's 5 to 6 only for us.
Thank you for explanation.

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 20, 2020

I tested ./gradlew install with trunk versus this PR after ./gradlew clean:

  • trunk: BUILD SUCCESSFUL in 1m 23s
  • this PR: BUILD SUCCESSFUL in 1m 50s

I think that's OK given the incremental compilation improvements.

@ijuma
Copy link
Contributor Author

@ijuma ijuma commented Apr 20, 2020

@omkreddy I've done a bunch of testing and this seems to work as expected generally.

One part that I wasn't completely sure about is the behavior of ./gradlew uploadArchivesAll in trunk versus ./gradlewAll uploadArchives in this PR with regards to Java modules that end up getting built twice due to being dependencies of the Scala modules (e.g kafka-clients, kafka-streams). When I try this in trunk with a local Maven repository, it fails. With the code in this PR, it completes, but these modules get uploaded twice.

So, I have decided to go ahead and merge this PR.

@ijuma ijuma merged commit d6da045 into apache:trunk Apr 20, 2020
1 of 2 checks passed
@ijuma ijuma deleted the gradle-6.0 branch Apr 20, 2020
ijuma added a commit to confluentinc/kafka-streams-examples that referenced this issue Apr 21, 2020
…invocation of `gradle`

See apache/kafka#7677 for more details with regards to
the former and apache/kafka#6031 with regards to the latter.
ijuma added a commit to confluentinc/kafka that referenced this issue Apr 21, 2020
* apache-github/trunk:
  MINOR: Fix grammar in error message for InvalidRecordException (apache#8465)
  KAFKA-9868: Reduce number of transaction log partitions for embed broker (apache#8522)
  MINOR: Adding github whitelist (apache#8523)
  MINOR: Upgrade gradle plugins and test libraries for Java 14 support (apache#8519)
  MINOR: Further reduce runtime for metrics integration tests (apache#8514)
  MINOR: Use .asf.yaml to direct github notifications to JIRA and mailing lists (apache#8521)
  MINOR: Update to Gradle 6.3 (apache#7677)
  HOTFIX: fix checkstyle error of RocksDBStoreTest and flaky RocksDBTimestampedStoreTest.shouldOpenExistingStoreInRegularMode (apache#8515)
  MINOR: cleanup RocksDBStore tests  (apache#8510)
  KAFKA-9818: Fix flaky test in RecordCollectorTest (apache#8507)
  MINOR: reduce impact of trace logging in replica hot path (apache#8468)
  KAFKA-6145: KIP-441: Add test scenarios to ensure rebalance convergence (apache#8475)
  KAFKA-9881: Convert integration test to verify measurements from RocksDB to unit test (apache#8501)
  MINOR: improve test coverage for dynamic LogConfig(s) (apache#7616)
  MINOR: Switch order of sections on tumbling and hopping windows in streams doc. Tumbling windows are defined as "special case of hopping time windows" - but hopping windows currently only explained later in the docs. (apache#8505)
  KAFKA-9819: Fix flaky test in StoreChangelogReaderTest (apache#8488)
ijuma added a commit to confluentinc/kafka-streams-examples that referenced this issue Apr 21, 2020
…invocation of `gradle` (#329)

See apache/kafka#7677 for more details with regards to
the former and apache/kafka#6031 with regards to the latter.
ijuma added a commit that referenced this issue Jun 3, 2020
Gradle 6.5 includes a fix for gradle/gradle#12866, which
affects the performance of Scala compilation.

I profiled the scalac build with async profiler and 54% of the time was on GC
even after the Gradle upgrade (it was more than 60% before), so I switched to
the throughput GC (GC latency is less important for batch builds) and it
was reduced to 38%.

I also centralized the jvm configuration in `build.gradle` and simplified it a bit
by removing the minHeapSize configuration from the test tasks.

On my desktop, the time to execute clean builds with no cached Gradle daemon
was reduced from 127 seconds to 97 seconds. With a cached daemon, it was
reduced from 120 seconds to 88 seconds. The performance regression when
we upgraded to Gradle 6.x was 27 seconds with a cached daemon 
(#7677 (comment)), so it
should be fixed now.

Gradle 6.4 with no cached daemon:

```
BUILD SUCCESSFUL in 2m 7s
115 actionable tasks: 112 executed, 3 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.15s user 0.12s system 0% cpu 2:08.06 total
```

Gradle 6.4 with cached daemon:

```
BUILD SUCCESSFUL in 2m 0s
115 actionable tasks: 111 executed, 4 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  0.95s user 0.10s system 0% cpu 2:01.42 total
```

Gradle 6.5 with no cached daemon:

```
BUILD SUCCESSFUL in 1m 46s
115 actionable tasks: 111 executed, 4 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.27s user 0.12s system 1% cpu 1:47.71 total
```

Gradle 6.5 with cached daemon:

```
BUILD SUCCESSFUL in 1m 37s
115 actionable tasks: 111 executed, 4 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.02s user 0.10s system 1% cpu 1:38.31 total
```

This PR with no cached Gradle daemon:

```
BUILD SUCCESSFUL in 1m 37s
115 actionable tasks: 81 executed, 34 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.27s user 0.10s system 1% cpu 1:38.70 total
```

This PR with cached Gradle daemon:

```
BUILD SUCCESSFUL in 1m 28s
115 actionable tasks: 111 executed, 4 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.02s user 0.10s system 1% cpu 1:29.35 total
```

Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
ijuma added a commit that referenced this issue Jun 3, 2020
Gradle 6.5 includes a fix for gradle/gradle#12866, which
affects the performance of Scala compilation.

I profiled the scalac build with async profiler and 54% of the time was on GC
even after the Gradle upgrade (it was more than 60% before), so I switched to
the throughput GC (GC latency is less important for batch builds) and it
was reduced to 38%.

I also centralized the jvm configuration in `build.gradle` and simplified it a bit
by removing the minHeapSize configuration from the test tasks.

On my desktop, the time to execute clean builds with no cached Gradle daemon
was reduced from 127 seconds to 97 seconds. With a cached daemon, it was
reduced from 120 seconds to 88 seconds. The performance regression when
we upgraded to Gradle 6.x was 27 seconds with a cached daemon 
(#7677 (comment)), so it
should be fixed now.

Gradle 6.4 with no cached daemon:

```
BUILD SUCCESSFUL in 2m 7s
115 actionable tasks: 112 executed, 3 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.15s user 0.12s system 0% cpu 2:08.06 total
```

Gradle 6.4 with cached daemon:

```
BUILD SUCCESSFUL in 2m 0s
115 actionable tasks: 111 executed, 4 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  0.95s user 0.10s system 0% cpu 2:01.42 total
```

Gradle 6.5 with no cached daemon:

```
BUILD SUCCESSFUL in 1m 46s
115 actionable tasks: 111 executed, 4 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.27s user 0.12s system 1% cpu 1:47.71 total
```

Gradle 6.5 with cached daemon:

```
BUILD SUCCESSFUL in 1m 37s
115 actionable tasks: 111 executed, 4 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.02s user 0.10s system 1% cpu 1:38.31 total
```

This PR with no cached Gradle daemon:

```
BUILD SUCCESSFUL in 1m 37s
115 actionable tasks: 81 executed, 34 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.27s user 0.10s system 1% cpu 1:38.70 total
```

This PR with cached Gradle daemon:

```
BUILD SUCCESSFUL in 1m 28s
115 actionable tasks: 111 executed, 4 up-to-date
./gradlew clean compileScala compileJava compileTestScala compileTestJava  1.02s user 0.10s system 1% cpu 1:29.35 total
```

Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants