Build: Fix flaky checkstyle issue #7321
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc: @nastra, @Fokko, @stevenzwu, @jackye1995 , @XN137 |
|
Oops. This was perfectly compiled locally. But failed in the CI build. |
ajantha-bhat
changed the title
nastra
reviewed
Apr 11, 2023
baseline.gradle
Outdated
| apply plugin: 'com.palantir.baseline-checkstyle' | ||
| // com.palantir.baseline:gradle-baseline-java:4.42.0 (the last version supporting Java 8) pulls | ||
| // in an old version of the checkstyle(9.1), which has this OutOfMemory bug https://github.com/checkstyle/checkstyle/issues/10934 | ||
| // So, replace com.palantir.baseline-checkstyle plugin usage with gradle checkstyle plugin |
There was a problem hiding this comment.
So, replace "com.palantir.baseline-checkstyle" with "checkstyle" 9.3 (the latest java 8 supported version) which contains a fix
nastra
reviewed
Apr 12, 2023
| @@ -124,10 +125,12 @@ private org.apache.avro.Schema fixupAvroSchemaConvertedFromIcebergSchema( | |||
| LogicalTypes.timeMillis() | |||
| .addToSchema( | |||
| org.apache.avro.Schema.create(org.apache.avro.Schema.Type.INT)); | |||
| field = new org.apache.avro.Schema.Field("time_field", fieldSchema); | |||
| updatedField = new org.apache.avro.Schema.Field("time_field", fieldSchema); | |||
| } else { | |||
There was a problem hiding this comment.
maybe init updatedField = field at the beginning rather than having an else
|
PR is ready for review. |
nastra
approved these changes
Apr 12, 2023
szehon-ho
approved these changes
Apr 12, 2023
1 task
|
Merged, let's see if this fixes it, thanks @ajantha-bhat for investigation, and @nastra for review |
manisin
pushed a commit
to Snowflake-Labs/iceberg
that referenced
this pull request
May 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#7024 didn't really fix the issue.
After analyzing the dependency tree found that
com.palantir.baseline-checkstyleplugin will always use the checkstyle version fromcom.palantir.baseline:gradle-baseline-java:4.42.0(which is 9.1) even though we override it with some other version (which is 9.3) using classpath.So, the solution is to override using the toolchain version of the
CheckstyleExtensionas supported fromcom.palantir.baseline-checkstyleinstead of classpath.https://github.com/palantir/gradle-baseline/blob/267dd19dad6dbcc9d9772437340b4abc72deda55/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineCheckstyle.java#L43-L45
gradle iceberg-api:dependenciesand confirmed it is using checkstyle 9.3 which has the fix of OOM issue which is causing build flakiness.fixes #7292