You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "ijuma (via GitHub)" <gi...@apache.org> on 2023/04/15 20:26:59 UTC
[GitHub] [kafka] ijuma opened a new pull request, #13582: MINOR: Fix lossy conversions flagged by Java 20
ijuma opened a new pull request, #13582:
URL: https://github.com/apache/kafka/pull/13582
An example of the warning:
> warning: [lossy-conversions] implicit cast from long to int in compound assignment is possibly lossy
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #13582:
URL: https://github.com/apache/kafka/pull/13582#discussion_r1236050527
##########
clients/src/main/java/org/apache/kafka/common/serialization/ShortDeserializer.java:
##########
@@ -34,7 +34,7 @@ public Short deserialize(String topic, byte[] data) {
short value = 0;
for (byte b : data) {
value <<= 8;
- value |= b & 0xFF;
+ value |= (short) (b & 0xFF);
Review Comment:
Fixed SpotBugs issue by changing `(byte)` to `(short)`.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #13582:
URL: https://github.com/apache/kafka/pull/13582#discussion_r1225487332
##########
clients/src/main/java/org/apache/kafka/common/record/CompressionType.java:
##########
@@ -128,11 +128,11 @@ public InputStream wrapForInput(ByteBuffer buffer, byte messageVersion, BufferSu
}
};
- public final int id;
+ public final byte id;
Review Comment:
Done.
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #13582:
URL: https://github.com/apache/kafka/pull/13582#discussion_r1170050264
##########
clients/src/main/java/org/apache/kafka/common/record/DefaultRecordBatch.java:
##########
@@ -434,7 +434,7 @@ private static byte computeAttributes(CompressionType type, TimestampType timest
if (isControl)
attributes |= CONTROL_FLAG_MASK;
if (type.id > 0)
- attributes |= COMPRESSION_CODEC_MASK & type.id;
+ attributes |= (byte) (COMPRESSION_CODEC_MASK & type.id);
Review Comment:
Unfortunately, the answer is yes: https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.6.2
--
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
[GitHub] [kafka] divijvaidya commented on pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13582:
URL: https://github.com/apache/kafka/pull/13582#issuecomment-1592854559
@ijuma are we waiting to get additional pair of eyes on this one? I am comfortable with merging this PR in it's current form.
--
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
[GitHub] [kafka] ijuma commented on pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #13582:
URL: https://github.com/apache/kafka/pull/13582#issuecomment-1600838939
Jobs run in parallel and hence don't add time. Daily jobs are basically ignored and are sort of useless. But we can discuss that in the relevant PR.
--
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
[GitHub] [kafka] divijvaidya commented on pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13582:
URL: https://github.com/apache/kafka/pull/13582#issuecomment-1600815777
When you are creating the CI build @ijuma perhaps, consider creating a daily job instead on every PR? Current CI takes 3.5 hours and this new stage may increase it further.
A daily run will help catch any regressions that we may have for JDK 20 until we want to officially support the next LTS release in Kakfa.
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13582:
URL: https://github.com/apache/kafka/pull/13582#discussion_r1170245494
##########
clients/src/main/java/org/apache/kafka/common/record/DefaultRecordBatch.java:
##########
@@ -434,7 +434,7 @@ private static byte computeAttributes(CompressionType type, TimestampType timest
if (isControl)
attributes |= CONTROL_FLAG_MASK;
if (type.id > 0)
- attributes |= COMPRESSION_CODEC_MASK & type.id;
+ attributes |= (byte) (COMPRESSION_CODEC_MASK & type.id);
Review Comment:
I expected type promotion to a common ancestor for cases where overflow is expected such as multiplication but didn't expect it for `&` bit operation. Nevertheless TIL!
--
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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13582:
URL: https://github.com/apache/kafka/pull/13582#discussion_r1169714114
##########
clients/src/main/java/org/apache/kafka/common/record/CompressionType.java:
##########
@@ -128,11 +128,11 @@ public InputStream wrapForInput(ByteBuffer buffer, byte messageVersion, BufferSu
}
};
- public final int id;
+ public final byte id;
Review Comment:
please add a comment that in the schema for RecordBatch, the type is represented by 2 bits. Hence, byte representation is sufficient.
##########
clients/src/main/java/org/apache/kafka/common/record/DefaultRecordBatch.java:
##########
@@ -434,7 +434,7 @@ private static byte computeAttributes(CompressionType type, TimestampType timest
if (isControl)
attributes |= CONTROL_FLAG_MASK;
if (type.id > 0)
- attributes |= COMPRESSION_CODEC_MASK & type.id;
+ attributes |= (byte) (COMPRESSION_CODEC_MASK & type.id);
Review Comment:
Do we still need this cast to byte? Both `COMPRESSION_CODEC_MASK` and `type.id` are bytes.
--
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
[GitHub] [kafka] ijuma merged pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma merged PR #13582:
URL: https://github.com/apache/kafka/pull/13582
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #13582:
URL: https://github.com/apache/kafka/pull/13582#discussion_r1236054069
##########
clients/src/main/java/org/apache/kafka/common/record/DefaultRecord.java:
##########
@@ -431,7 +431,7 @@ private static void skipBytes(InputStream in, int bytesToSkip) throws IOExceptio
// Starting JDK 12, this implementation could be replaced by InputStream#skipNBytes
while (bytesToSkip > 0) {
- long ns = in.skip(bytesToSkip);
+ int ns = (int) in.skip(bytesToSkip);
Review Comment:
New issue introduced in trunk after this PR was originally created. There are a few others, but I'll address them together with the JDK 20 CI build (which will prevent the issue altogether).
--
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
[GitHub] [kafka] ijuma commented on pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #13582:
URL: https://github.com/apache/kafka/pull/13582#issuecomment-1597375537
@divijvaidya if you're comfortable, I think this is ready to be merged.
--
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
[GitHub] [kafka] ijuma commented on pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #13582:
URL: https://github.com/apache/kafka/pull/13582#issuecomment-1602803318
After several test runs, it looks like the failures in the last run are flakes.
--
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
[GitHub] [kafka] ijuma commented on pull request #13582: MINOR: Fix lossy conversions flagged by Java 20
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #13582:
URL: https://github.com/apache/kafka/pull/13582#issuecomment-1597377059
Looks like there is a spotbugs issue, I'll investigate that.
--
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