You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mumrah (via GitHub)" <gi...@apache.org> on 2024/08/31 18:21:59 UTC
[PR] MINOR Fix gradle jar caching [kafka]
mumrah opened a new pull request, #17066:
URL: https://github.com/apache/kafka/pull/17066
For several modules, we include a `kafka-version.properties` in the Jar file. This file includes the Git SHA of the project at the time of the build. This means that even if no source files change, the `:jar` task will never be UP-TO-DATE between git commits. Ultimately, this breaks Gradle caching.
This patch marks all of the `createVersionFile` tasks as cacheable and also changes our Gradle invocation to override the commit ID to a dummy static value. This will allow the `:jar` task to be cacheable and reusable between builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17066:
URL: https://github.com/apache/kafka/pull/17066#discussion_r1744156948
##########
.github/workflows/build.yml:
##########
@@ -94,18 +99,22 @@ jobs:
with:
java-version: ${{ matrix.java }}
gradle-cache-read-only: ${{ inputs.gradle-cache-read-only }}
+ gradle-cache-write-only: ${{ inputs.gradle-cache-write-only }}
develocity-access-key: ${{ secrets.GE_ACCESS_TOKEN }}
- name: Test
# Gradle flags
# --build-cache: Let Gradle restore the build cache
# --scan: Attempt to publish build scans in PRs. This will only work on PRs from apache/kafka, not public forks.
# --continue: Keep running even if a test fails
+ # -PcommitId Prevent the Git SHA being written into the jar files (which breaks caching)
timeout-minutes: 180 # 3 hours
+ continue-on-error: true
run: |
./gradlew --build-cache --scan --continue \
-PtestLoggingEvents=started,passed,skipped,failed \
- -PignoreFailures=true -PmaxParallelForks=2 \
+ -PmaxParallelForks=2 \
-PmaxTestRetries=1 -PmaxTestRetryFailures=10 \
+ -PcommitId=xxxxxxxxxxxxxxxx \
Review Comment:
No, this is needed for the CI to avoid breaking the cache for the `:jar` and other downstream tasks. Otherwise, we could never cache things like kafka-clients.jar because its contents would change on every commit, regardless of changes to the client source files.
We really only need this kafka-version.properties file to include an actual Git SHA for release artifacts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR Fix gradle jar caching [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #17066:
URL: https://github.com/apache/kafka/pull/17066#issuecomment-2323047641
In this build we can see that all the tasks, including tests, are FROM-CACHE
https://github.com/mumrah/kafka/actions/runs/10648128235/job/29516852943
However, the test results were included in the cache, which is kind of questionable. Working on that next
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17066:
URL: https://github.com/apache/kafka/pull/17066#discussion_r1744071589
##########
build.gradle:
##########
@@ -882,6 +882,7 @@ project(':server') {
inputs.property "commitId", commitId
inputs.property "version", version
outputs.file receiptFile
+ outputs.cacheIf { true }
Review Comment:
That means the jars could have old `kafka-version.properties` when its module has no change. This change is for CI, right? we want to cache the jar if no source files change. Maybe we can enable the `cacheIf` when the env `GITHUB_ACTIONS` exists. Otherwise, we keep origin behavior. WDYT?
##########
.github/workflows/build.yml:
##########
@@ -94,18 +99,22 @@ jobs:
with:
java-version: ${{ matrix.java }}
gradle-cache-read-only: ${{ inputs.gradle-cache-read-only }}
+ gradle-cache-write-only: ${{ inputs.gradle-cache-write-only }}
develocity-access-key: ${{ secrets.GE_ACCESS_TOKEN }}
- name: Test
# Gradle flags
# --build-cache: Let Gradle restore the build cache
# --scan: Attempt to publish build scans in PRs. This will only work on PRs from apache/kafka, not public forks.
# --continue: Keep running even if a test fails
+ # -PcommitId Prevent the Git SHA being written into the jar files (which breaks caching)
timeout-minutes: 180 # 3 hours
+ continue-on-error: true
run: |
./gradlew --build-cache --scan --continue \
-PtestLoggingEvents=started,passed,skipped,failed \
- -PignoreFailures=true -PmaxParallelForks=2 \
+ -PmaxParallelForks=2 \
-PmaxTestRetries=1 -PmaxTestRetryFailures=10 \
+ -PcommitId=xxxxxxxxxxxxxxxx \
Review Comment:
this is for testing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #17066:
URL: https://github.com/apache/kafka/pull/17066#issuecomment-2329139782
This has been fixed and verified over at https://github.com/mumrah/kafka/actions/runs/10693770259.
We'll merge it in two parts. The first part is this PR which fixes the ":jar" task and makes trunk use the `gradle-cache-write-only`. Once this is merged and the cache has been populated by the trunk build, a part 2 PR can be opened that verifies the remaining changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah merged PR #17066:
URL: https://github.com/apache/kafka/pull/17066
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17066:
URL: https://github.com/apache/kafka/pull/17066#discussion_r1743844085
##########
.github/scripts/junit.py:
##########
@@ -55,6 +55,9 @@ class TestCase:
def key(self) -> Tuple[str, str]:
return self.class_name, self.test_name
+ def __repr__(self):
+ return f"{self.class_name} {self.test_name}"
Review Comment:
Unrelated to this PR, but was causing issues in some cases where a test stacktrace was really long
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17066:
URL: https://github.com/apache/kafka/pull/17066#discussion_r1744153701
##########
build.gradle:
##########
@@ -882,6 +882,7 @@ project(':server') {
inputs.property "commitId", commitId
inputs.property "version", version
outputs.file receiptFile
+ outputs.cacheIf { true }
Review Comment:
`outputs.cacheIf` is one way to tell Gradle that a task is cacheable https://docs.gradle.org/current/userguide/build_cache.html#using_the_runtime_api.
Even with this declaration, the task will only be up-to-date / cacheable if the inputs are unchanged.
Taking the previous task definition:
```
task createVersionFile() {
inputs.property "commitId", commitId
inputs.property "version", version
outputs.file receiptFile
```
For incremental builds, this task will be considered UP-TO-DATE as long as the two inputs don't change. In order to actually allow this output to be _cached_, we need to tell gradle that it's cacheable. That's done by adding the `@CacheableTask` for custom task classes, or by adding `outputs.cacheIf { true }`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17066:
URL: https://github.com/apache/kafka/pull/17066#discussion_r1744153701
##########
build.gradle:
##########
@@ -882,6 +882,7 @@ project(':server') {
inputs.property "commitId", commitId
inputs.property "version", version
outputs.file receiptFile
+ outputs.cacheIf { true }
Review Comment:
`outputs.cacheIf` is one way to tell Gradle that a task is cacheable https://docs.gradle.org/current/userguide/build_cache.html#using_the_runtime_api.
Even with this declaration, the task will only be up-to-date / cacheable if the inputs are unchanged.
Taking the previous task definition:
```
task createVersionFile() {
inputs.property "commitId", commitId
inputs.property "version", version
outputs.file receiptFile
```
For incremental builds, this task will be considered UP-TO-DATE as long as the two inputs don't change. In order to actually allow this output to be _cached_, we need to tell gradle that it's cacheable. That's done by adding the `@CacheableTask` for custom task classes, or by adding `outputs.cacheIf { true }`
Edit: sorry, I misinterpreted your question I think. I'll leave the original comment here anyways.
> That means the jars could have old kafka-version.properties when its module has no change
The contents of kafka-version.properties will be the same because we're overriding the commit ID with `-PcommitId=xxxxxxxxxxxxxxxx`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17066:
URL: https://github.com/apache/kafka/pull/17066#discussion_r1744153701
##########
build.gradle:
##########
@@ -882,6 +882,7 @@ project(':server') {
inputs.property "commitId", commitId
inputs.property "version", version
outputs.file receiptFile
+ outputs.cacheIf { true }
Review Comment:
`outputs.cacheIf` is one way to tell Gradle that a task is cacheable https://docs.gradle.org/current/userguide/build_cache.html#using_the_runtime_api.
Even with this declaration, the task will only be up-to-date / cacheable if the inputs are unchanged.
So, taking the previous task definition:
```
task createVersionFile() {
inputs.property "commitId", commitId
inputs.property "version", version
outputs.file receiptFile
```
For incremental builds, this task will be considered UP-TO-DATE as long as the two inputs don't change. In order to actually allow this output to be _cached_, we need to tell gradle that it's cacheable. That's done by adding the `@CacheableTask` for custom task classes, or by adding `outputs.cacheIf { true }`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17479 Allow ":jar" tasks to be cached [1/n] [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #17066:
URL: https://github.com/apache/kafka/pull/17066#discussion_r1743859331
##########
.github/workflows/build.yml:
##########
@@ -94,18 +99,22 @@ jobs:
with:
java-version: ${{ matrix.java }}
gradle-cache-read-only: ${{ inputs.gradle-cache-read-only }}
+ gradle-cache-write-only: ${{ inputs.gradle-cache-write-only }}
develocity-access-key: ${{ secrets.GE_ACCESS_TOKEN }}
- name: Test
# Gradle flags
# --build-cache: Let Gradle restore the build cache
# --scan: Attempt to publish build scans in PRs. This will only work on PRs from apache/kafka, not public forks.
# --continue: Keep running even if a test fails
+ # -PcommitId Prevent the Git SHA being written into the jar files (which breaks caching)
timeout-minutes: 180 # 3 hours
+ continue-on-error: true
run: |
./gradlew --build-cache --scan --continue \
-PtestLoggingEvents=started,passed,skipped,failed \
- -PignoreFailures=true -PmaxParallelForks=2 \
Review Comment:
`-PignoreFailures=true` causes Gradle to consider the build successful, even if a test task fails. This means that failing tests would be cached, which we don't want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR Fix gradle jar caching [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #17066:
URL: https://github.com/apache/kafka/pull/17066#issuecomment-2323020465
I am going to verify this fix over at mumrah/kafka
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org