-
Notifications
You must be signed in to change notification settings - Fork 3k
Build: Enable debugging for java-ci #6982
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
Conversation
|
cc: @Fokko, @nastra, @jackye1995, @XN137 |
.github/workflows/java-ci.yml
Outdated
| distribution: zulu | ||
| java-version: 8 | ||
| - run: ./gradlew -DallVersions build -x test -x javadoc -x integrationTest | ||
| - run: ./gradlew -DallVersions build -x test -x javadoc -x integrationTest --scan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesnt this require to agree to gradle's terms of service?
see https://docs.gradle.com/enterprise/gradle-plugin/#connecting_to_scans_gradle_com
imo if the sporadic errors always happen in checkstyle part, we may want to check if we can upgrade the respective plugin to fix the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesnt this require to agree to gradle's terms of service
good point, in that case we probably cannot enable it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think other projects like Nessie Agree to these terms of service. I have skimmed it and didn't find anything harmful.
Do you guys think we should not enable --scan and just enable --stacktrace --debug ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo if the sporadic errors always happen in checkstyle part, we may want to check if we can upgrade the respective plugin to fix the issue
we are using the latest version I assume as I didn't find any version pinning.
https://github.com/apache/iceberg/blob/master/baseline.gradle#L36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are only using latest version of baseline which supported java 8:
Lines 30 to 34 in 35692a3
| classpath 'com.palantir.baseline:gradle-baseline-java:4.42.0' | |
| // com.palantir.baseline:gradle-baseline-java:4.42.0 (the last version supporting Java 8) pulls | |
| // in an old version of the errorprone, which doesn't work w/ Gradle 8, so bump errorpone as | |
| // well. | |
| classpath "net.ltgt.gradle:gradle-errorprone-plugin:3.0.1" |
in there we can find the checkstyle version:
https://github.com/palantir/gradle-baseline/blob/4.42.0/versions.props#L17
but i think 9.3 would be the last version with java8 support:
https://checkstyle.org/releasenotes.html#Release_9.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more of a licensing issue, not sure if there is any other project under Apache Foundation using this feature, we can ping the infra team about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True but the gradle-baseline-4.42.0 may not accept new patches :(
Also what is your opinion on
Do you guys think we should not enable --scan and just enable --stacktrace --debug ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to have the failure reason before raising the patch to the gradle-baseline. So, I believe we need to get stack trace/debug first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before raising the patch to the gradle-baseline
i was trying to say there would probably be ways to bump the checkstyle version without needing to get a new gradle-baseline version
that being said, i agree we should try to get stacktrace first to see what exactly goes wrong occasionally
|
@XN137 and @jackye1995: Thanks for the review. I have changed to |
|
Rather than merging this PR with debugging enabled, I'd rather vote for rerunning CI on this PR until we run into the issue, as otherwise this will be spamming logs. That being said, I'm leaning towards -1 on merging this. Also it might actually leak security information according to Gradle: |
|
changed to |
|
no luck in reproducing yet! |
|
Discovered a new flaky test 🤦 |
|
I rebased and pushed to this branch when the PR was closed. So, I can't reopen the PR. will open a new PR again to debug this. |
Too many times, I am facing one flaky error (also other people are facing it too). Unable to know the reason. Hence, enabling the
--stacktrace,--infofor future debugging.Failure details:
https://github.com/apache/iceberg/actions/workflows/java-ci.yml
https://github.com/apache/iceberg/actions/runs/4309428443
https://github.com/apache/iceberg/actions/runs/4310045826
https://github.com/apache/iceberg/actions/runs/4306392493/
and many more