You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "abhishekrb19 (via GitHub)" <gi...@apache.org> on 2025/11/05 17:06:58 UTC
[PR] [WIP]: Trigger JDK 21 tests (druid)
abhishekrb19 opened a new pull request, #18717:
URL: https://github.com/apache/druid/pull/18717
The JDK 21 tests don't seem to run by default. One of the `CompressionUtilsTest`s is failing in master, so open this to triage.
See related slack thread - https://apachedruidworkspace.slack.com/archives/C030CMF6B70/p1762309385094569
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] [WIP]: Trigger JDK 21 tests (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18717:
URL: https://github.com/apache/druid/pull/18717#discussion_r2495509151
##########
processing/src/test/java/org/apache/druid/java/util/common/CompressionUtilsTest.java:
##########
@@ -602,7 +602,7 @@
}
};
- Assert.assertEquals((long) (EXPECTED.length * 3), CompressionUtils.gunzip(inputStreamFactory, testFile).size());
+ Assert.assertEquals(EXPECTED.length * 3, CompressionUtils.gunzip(inputStreamFactory, testFile).size());
Review Comment:
## Result of multiplication cast to wider type
Potential overflow in [int multiplication](1) before it is converted to long by use in an invocation context.
[Show more details](https://github.com/apache/druid/security/code-scanning/5759)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Run JDK 21 tests on PRs (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #18717:
URL: https://github.com/apache/druid/pull/18717
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Run JDK 21 tests on PRs (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18717:
URL: https://github.com/apache/druid/pull/18717#discussion_r2505693482
##########
.github/workflows/ci.yml:
##########
@@ -71,21 +72,6 @@ jobs:
artifacts_to_download: "unit-test-reports-*"
key: "coverage-jacoco"
- run-unit-tests-all-jdk:
Review Comment:
Ah, got it. In that case running both seems fine.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Run JDK 21 tests on PRs (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18717:
URL: https://github.com/apache/druid/pull/18717#discussion_r2505273648
##########
.github/workflows/ci.yml:
##########
@@ -71,21 +72,6 @@ jobs:
artifacts_to_download: "unit-test-reports-*"
key: "coverage-jacoco"
- run-unit-tests-all-jdk:
Review Comment:
In the interests of making CI run faster, IMO it would be good to keep this, but flip 17 and 21. So, every PR runs java 21 tests and some run 17 tests. That way we're always doing our regular testing with the latest supported JDK. There is a tradeoff here in terms of catching stuff earlier, but I tend to believe that CI running quickly is worth the tradeoff.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Run JDK 21 tests on PRs (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #18717:
URL: https://github.com/apache/druid/pull/18717#discussion_r2517753467
##########
processing/src/test/java/org/apache/druid/java/util/common/CompressionUtilsTest.java:
##########
@@ -551,8 +552,9 @@ public void testZeroRemainingInputStream() throws IOException
}
// If this ever passes, er... fails to fail... then the bug is fixed
- @Test(expected = AssertionError.class)
// http://bugs.java.com/bugdatabase/view_bug.do?bug_id=7036144
+ @Test(expected = AssertionError.class)
+ @Ignore("This test fails in JDK 21, looks like the bug was fixed in 21.0.9 at least.")
Review Comment:
couldn't you skip this testcase in a jdk version aware way?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] [WIP]: Trigger JDK 21 tests (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 closed pull request #18717: [WIP]: Trigger JDK 21 tests
URL: https://github.com/apache/druid/pull/18717
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Run JDK 21 tests on PRs (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #18717:
URL: https://github.com/apache/druid/pull/18717#discussion_r2505385491
##########
.github/workflows/ci.yml:
##########
@@ -71,21 +72,6 @@ jobs:
artifacts_to_download: "unit-test-reports-*"
key: "coverage-jacoco"
- run-unit-tests-all-jdk:
Review Comment:
The unit tests seem to run in parallel for the matrix entries - one with [both 17 and 21 ](https://github.com/apache/druid/actions/runs/19174393128) in this PR took ~1h 13m and another PR with only [17](https://github.com/apache/druid/actions/runs/19166672258) took ~1h 12m (here's another one with [only](https://github.com/apache/druid/actions/runs/19010125601) 17 that took ~1h 5m). Unless we’re hitting a concurrency limit on GHA jobs, this shouldn’t be a problem purely for CI runtime?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Run JDK 21 tests on PRs (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on PR #18717:
URL: https://github.com/apache/druid/pull/18717#issuecomment-3521159219
the purpose of it was to run the tests once on the primary jdk; and optional only then proceed to others (possibly reducing resource use if there are failed tests; note: around when this was made testing was run on 3 jdks)
alternate jdk runs were deliberatlely skipped on PRs which were not requesting for it via the label - to reduce resource usage as in most cases these wont add further coverage to a PR.
the current approach parallelizes the 2 jdk results and merges them in the test-report - which may make it less obvious if a test fails only with 1 jdk
ideally the alternate jdk runs should run in parallel with the integration test like things...but never got to that
I would like to highlight that in case of PR testing its important to provide results for possible multiple PRs - optimizing to use up resources to service a single PR will not be the best use of resources as it could force the system to queue up processing earlier.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org