You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/11/19 00:19:51 UTC
[GitHub] [iceberg] dchristle opened a new pull request, #6221: Change SingleBufferInputStream .read signature to match super-method.
dchristle opened a new pull request, #6221:
URL: https://github.com/apache/iceberg/pull/6221
**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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko merged pull request #6221: Change SingleBufferInputStream .read signature to match super-method.
Posted by GitBox <gi...@apache.org>.
Fokko merged PR #6221:
URL: https://github.com/apache/iceberg/pull/6221
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6221: Change SingleBufferInputStream .read signature to match super-method.
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #6221:
URL: https://github.com/apache/iceberg/pull/6221#issuecomment-1337501228
> 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 is also affected by this.
thanks @Fokko for merging the PR and @dchristle for the fix.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6221: Change SingleBufferInputStream .read signature to match super-method.
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #6221:
URL: https://github.com/apache/iceberg/pull/6221#issuecomment-1330599222
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 releases.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #6221: Change SingleBufferInputStream .read signature to match super-method.
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #6221:
URL: https://github.com/apache/iceberg/pull/6221#issuecomment-1330471975
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] XN137 commented on pull request #6221: Change SingleBufferInputStream .read signature to match super-method.
Posted by GitBox <gi...@apache.org>.
XN137 commented on PR #6221:
URL: https://github.com/apache/iceberg/pull/6221#issuecomment-1337298037
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 jdk11 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 :sweat_drops:
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org