[GitHub] [kafka] divijvaidya commented on a diff in pull request #12948: MINOR: Add JDK 20 CI build and remove some branch builds
divijvaidya commented on code in PR #12948:
URL: https://github.com/apache/kafka/pull/12948#discussion_r1242377704
##
clients/src/main/java/org/apache/kafka/common/utils/ChunkedBytesStream.java:
##
@@ -290,25 +290,27 @@ public long skip(long toSkip) throws IOException {
// Skip bytes stored in intermediate buffer first
int avail = count - pos;
-long bytesSkipped = (avail < remaining) ? avail : remaining;
+int bytesSkipped = Math.min(avail, (int) remaining);
Review Comment:
> It does show a gap in the tests for this class, we should ideally have
tests that cover these boundary cases.
Fair point. Added a JIRA https://issues.apache.org/jira/browse/KAFKA-15123
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12948: MINOR: Add JDK 20 CI build and remove some branch builds
divijvaidya commented on code in PR #12948:
URL: https://github.com/apache/kafka/pull/12948#discussion_r1242328144
##
clients/src/main/java/org/apache/kafka/common/utils/ChunkedBytesStream.java:
##
@@ -290,25 +290,27 @@ public long skip(long toSkip) throws IOException {
// Skip bytes stored in intermediate buffer first
int avail = count - pos;
-long bytesSkipped = (avail < remaining) ? avail : remaining;
+int bytesSkipped = Math.min(avail, (int) remaining);
Review Comment:
I am concerned about cases where `remaining` is outside the range of
representable `int` values, let's say `remaining = Integer.MIN_VALUE + 1000`.
When down casting in cases of overflow (i.e. in `Math.min(avail, (int)
remaining)`), the behaviour is implementation defined and numeric value for
`(int)remaining` may end up leading to a negative value numeric int.
Calculating min for this negative value with avail will lead to bytesSkipped as
negative.
In the implementation I suggested, the result of `Math.min((long) avail,
remaining)` is guaranteed to fit in `int` because it's upper bound by `avail`.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12948: MINOR: Add JDK 20 CI build and remove some branch builds
divijvaidya commented on code in PR #12948:
URL: https://github.com/apache/kafka/pull/12948#discussion_r1242092544
##
Jenkinsfile:
##
@@ -155,79 +155,23 @@ pipeline {
echo 'Skipping Kafka Streams archetype test for Java 17'
}
}
-
-// To avoid excessive Jenkins resource usage, we only run the stages
-// above at the PR stage. The ones below are executed after changes
-// are pushed to trunk and/or release branches. We achieve this via
-// the `when` clause.
-
-stage('JDK 8 and Scala 2.13') {
Review Comment:
Adding this note here for future visitors to this PR (and to ensure that we
are on the same page here about the reason to remove these stages from CI)
Removing this combination from tests in fine because scala 2.13 is fully
compatible with JDK 8 [1]. If we can guarantee that code is compiling correctly
with 2.13 and JDK 8 separately, then that is sufficient and we do not
necessarily have to test a unique combination of both.
[1] https://docs.scala-lang.org/overviews/jdk-compatibility/overview.html
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12948: MINOR: Add JDK 20 CI build and remove some branch builds
divijvaidya commented on code in PR #12948:
URL: https://github.com/apache/kafka/pull/12948#discussion_r1242078745
##
clients/src/main/java/org/apache/kafka/common/utils/ChunkedBytesStream.java:
##
@@ -290,25 +290,27 @@ public long skip(long toSkip) throws IOException {
// Skip bytes stored in intermediate buffer first
int avail = count - pos;
-long bytesSkipped = (avail < remaining) ? avail : remaining;
+int bytesSkipped = Math.min(avail, (int) remaining);
Review Comment:
I am not sure that this is a good idea to downcast and then perform
comparison. Comparison amongst different types is performed by upgrading them
to the higher type and then performing comparison. In current approach, we
downcasting `remaining` to int is unsafe.
May I suggest an alternative where we upgrade avail to `long` before
performing comparison and then converting result to int:` int bytesSkipped =
(int) Math.min((long) avail, remaining);`
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12948: MINOR: Add JDK 20 CI build and remove some branch builds
divijvaidya commented on code in PR #12948:
URL: https://github.com/apache/kafka/pull/12948#discussion_r1237418125
##
clients/src/main/java/org/apache/kafka/common/utils/ChunkedBytesStream.java:
##
@@ -291,7 +291,7 @@ public long skip(long toSkip) throws IOException {
// Skip bytes stored in intermediate buffer first
int avail = count - pos;
long bytesSkipped = (avail < remaining) ? avail : remaining;
-pos += bytesSkipped;
+pos += (int) bytesSkipped;
Review Comment:
I am afraid that won't be possible.
This is because there are two branches where `bytesSkipped` is updated. In
one branch when we call `bytesSkipped = getInIfOpen().skip(remaining)`,
`bytesSkipped` holds a long value since `getInIfOpen().skip(remaining)` returns
a long. In the other branch, when we call `bytesSkipped = (avail < remaining) ?
avail : remaining`, `bytesSkipped` holds an int since this min statement bounds
it to max of avail, which is an int.
I can attempt to do a bunch of refactoring if that will make things clearer
but that shouldn't block this 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12948: MINOR: Add JDK 20 CI build and remove some branch builds
divijvaidya commented on code in PR #12948:
URL: https://github.com/apache/kafka/pull/12948#discussion_r1237341025
##
clients/src/main/java/org/apache/kafka/common/utils/ChunkedBytesStream.java:
##
@@ -291,7 +291,7 @@ public long skip(long toSkip) throws IOException {
// Skip bytes stored in intermediate buffer first
int avail = count - pos;
long bytesSkipped = (avail < remaining) ? avail : remaining;
-pos += bytesSkipped;
+pos += (int) bytesSkipped;
Review Comment:
@ijuma casting to int here is safe. Can you please add the following comment
here:
```
// cast to int is fine here since the value of bytesSkipped is upper bound
by value of avail
// which is an int
```
It is safe because as per the lines above it `long bytesSkipped = (avail <
remaining) ? avail : remaining;`, bytesSkipped is minimum of avail or remaining
which means it's max value is available which we know is an int.
Same for the other place.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12948: MINOR: Add JDK 20 CI build and remove some branch builds
divijvaidya commented on code in PR #12948:
URL: https://github.com/apache/kafka/pull/12948#discussion_r1237149655
##
clients/src/main/java/org/apache/kafka/common/utils/ChunkedBytesStream.java:
##
@@ -291,7 +291,7 @@ public long skip(long toSkip) throws IOException {
// Skip bytes stored in intermediate buffer first
int avail = count - pos;
long bytesSkipped = (avail < remaining) ? avail : remaining;
-pos += bytesSkipped;
+pos += (int) bytesSkipped;
Review Comment:
I am on it
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
