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

Change SingleBufferInputStream .read signature to match super-method. #6221

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

dchristle
Copy link
Contributor

@dchristle dchristle commented Nov 19, 2022

What does this PR do?
Changes the variable name offset to off in the SingleBufferInputStream function.

Why are these changes needed?
When running ./gradlew build with sourceCompatibility = '11' and targetCompatibility = '11' instead of 1.8, the build is blocked by a ConsistentOverrides error:

iceberg/core/src/main/java/org/apache/iceberg/io/SingleBufferInputStream.java:67: error: [ConsistentOverrides] Method overrides should have variable names consistent with the super-method when there are multiple parameters with the same type to avoid incorrectly binding values to variables.
  public int read(byte[] bytes, int offset, int len) throws IOException {
             ^
    (see https://github.com/palantir/gradle-baseline#baseline-error-prone-checks)
  Did you mean 'public int read(byte[] bytes, int off, int len) throws IOException {'?
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
1 error

SingleBufferInputStream overrides superclass InputStream, where the equivalent method is:

public int read(byte[] b, int off, int len) throws IOException

and changing it to match the super-method fixes the error.

How were these changes tested?

  • ./gradlew build succeeds on sourceCompatibility = '11' and targetCompatibility = '11'.
  • ./gradlew build succeeds on sourceCompatibility = '1.8' and targetCompatibility = '1.8'.
  • Pass CI checks.

@github-actions github-actions bot added the core label Nov 19, 2022
@ajantha-bhat
Copy link
Member

I just bumped into the same issue when I rebased my fork to 1.1.0 release tag.
This issue can be reproduced by changing these https://github.com/apache/iceberg/blob/master/build.gradle#L153-L154 to 11 instead of 1.8

cc: @rdblue, @Fokko, @nastra , @gaborkaszab
I am not sure whether we need a new release just for this fix (1.1.1). I don't need a release immediately as I can cherry-pick this fix.

But we do need to merge this.

Copy link
Contributor

@nastra nastra left a 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 ok to update this, but I don't see a reason to do another release because Iceberg is being compiled+released with JDK 8 compatibility and I don't think this will change in the near future (due to Hive requiring JDK8)

@ajantha-bhat
Copy link
Member

ajantha-bhat commented Nov 29, 2022

Surprisingly I didn't modify source and target compatibility in my code. (using default 1.8). But I still get this error only from my jenkins build (not from local builds). Might have to do with java version used in my jenkins build environment or a bug in new versions of error-prone (unlikely). I need to dig further on this. The same stuff was working with previous iceberg releases.

@XN137
Copy link
Contributor

XN137 commented Dec 5, 2022

additional context:
the failing check can only be performed when the compiled bytecode contains parameter name information for methods:

https://github.com/palantir/gradle-baseline/blob/fd7759a9a37112db69f6875966e869a078a00319/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ConsistentOverrides.java#L151-L164

so it depends on whether the jdk classes were compiled with the -parameters flag for javac or not:

  -parameters
        Generate metadata for reflection on method parameters

on jdk 8 this flag is commonly not used:

~/.sdkman/candidates/java/8.0.352-zulu/bin$ ./javap -version && ./javap -l java.io.InputStream | egrep "public.*read|off"
1.8.0_352
  public abstract int read() throws java.io.IOException;
  public int read(byte[]) throws java.io.IOException;
  public int read(byte[], int, int) throws java.io.IOException;

on jdk 11 it is commonly used:

~/.sdkman/candidates/java/11.0.17-zulu/bin$ ./javap -version && ./javap -l java.io.InputStream | egrep "public.*read|off"
11.0.17
  public abstract int read() throws java.io.IOException;
  public int read(byte[]) throws java.io.IOException;
  public int read(byte[], int, int) throws java.io.IOException;
          0      81     2   off   I
  public byte[] readAllBytes() throws java.io.IOException;
  public byte[] readNBytes(int) throws java.io.IOException;
        200      74     7 offset   I
  public int readNBytes(byte[], int, int) throws java.io.IOException;
          0      53     2   off   I

but there are some jdk vendors (i.e. on centos7) that also enable this flag on jdk8, thus this check might fail depending on the jdk distribution used.
so targeting java 11 is just a way to always trigger the check violation.

long story short, this PR should get merged to fix the check on all kind of jdks 😅

@nastra nastra requested a review from Fokko December 5, 2022 14:10
@Fokko Fokko merged commit 2ecce26 into apache:master Dec 5, 2022
@ajantha-bhat
Copy link
Member

ajantha-bhat commented Dec 5, 2022

but there are some jdk vendors (i.e. on centos7) that also enable this flag on jdk8, thus this check might fail depending on the jdk distribution used.

@XN137: Thank you so much for digging down on this and figuring out how jdk8 (of some vendors) is also affected by this.

thanks @Fokko for merging the PR and @dchristle for the fix.

@dchristle dchristle deleted the dchristle/signature_fix branch December 5, 2022 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants