You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "Akshat-Jain (via GitHub)" <gi...@apache.org> on 2024/11/11 04:23:41 UTC
[PR] [WIP] Deprecate Java 8 (druid)
Akshat-Jain opened a new pull request, #17466:
URL: https://github.com/apache/druid/pull/17466
### Description
Will fill PR description when PR is ready for review.
#### Release note
todo
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [x] 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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846560433
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
I tried exploring different versions for this.
If we don't explicitly specify a version, it uses `3.2.0` from the parent, which runs into the following issue:
```
<pmd xmlns="http://pmd.sourceforge.net/report/2.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://pmd.sourceforge.net/report/2.0.0 http://pmd.sourceforge.net/report_2_0_0.xsd" version="6.42.0" timestamp="2024-11-18T17:56:13.310">
<error filename="/Users/akjn/code/druid/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java" msg="PMDException: Error while parsing /Users/akjn/code/druid/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java">
<![CDATA[ net.sourceforge.pmd.PMDException: Error while parsing /Users/akjn/code/druid/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java at net.sourceforge.pmd.SourceCodeProcessor.processSourceCodeWithoutCache(SourceCodeProcessor.java:124) at net.sourceforge.pmd.SourceCodeProcessor.processSourceCode(SourceCodeProcessor.java:100) at net.sourceforge.pmd.SourceCodeProcessor.processSourceCode(SourceCodeProcessor.java:62) at net.sourceforge.pmd.processor.PmdRunnable.call(PmdRunnable.java:85) at net.sourceforge.pmd.processor.PmdRunnable.call(PmdRunnable.java:29) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadP
oolExecutor.java:635) at java.base/java.lang.Thread.run(Thread.java:840) Caused by: net.sourceforge.pmd.lang.java.ast.ParseException: Encountered " "." ". "" at line 71, column 40. Was expecting one of: <IDENTIFIER> ... "@" ... "@" ... "@" ... "@" ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... <IDENTIFIER> ... at net.sourceforge.pmd.lang.java.ast.JavaParser.generateParseException(JavaParser.java:13532) at net.sourceforge.pmd.lang.java.ast.JavaParser.jj_consume_token(JavaParser.java:13416) at net.sourceforge.pmd.lang.java.ast.JavaParser.MethodDeclarator(JavaParser.java:2169) at net.sourceforge.pmd.lang.java.ast.JavaParser.MethodDeclaration(JavaParser.java:2118) at net.sourceforge.pmd.lang.java.ast.JavaParser.ClassOrInterfaceBodyDeclaration(JavaParser.java:1811) at net.sourceforge.pmd.lang.jav
a.ast.JavaParser.ClassOrInterfaceBody(JavaParser.java:1764) at net.sourceforge.pmd.lang.java.ast.JavaParser.ClassOrInterfaceDeclaration(JavaParser.java:905) at net.sourceforge.pmd.lang.java.ast.JavaParser.TypeDeclaration(JavaParser.java:807) at net.sourceforge.pmd.lang.java.ast.JavaParser.CompilationUnit(JavaParser.java:527) at net.sourceforge.pmd.lang.java.AbstractJavaParser.parse(AbstractJavaParser.java:62) at net.sourceforge.pmd.lang.AbstractParser.doParse(AbstractParser.java:44) at net.sourceforge.pmd.SourceCodeProcessor.parse(SourceCodeProcessor.java:136) at net.sourceforge.pmd.SourceCodeProcessor.processSource(SourceCodeProcessor.java:200) at net.sourceforge.pmd.SourceCodeProcessor.processSourceCodeWithoutCache(SourceCodeProcessor.java:118) ... 10 more ]]>
</error>
</pmd>
```
The above can be simulated using:
```
mvn dependency:analyze -DoutputXML=true -DignoreNonCompile=true -DfailOnWarning=true -X
```
The above is happening for the following line: https://github.com/apache/druid/blob/24a1fafaa745fb93bf1522490c095cd689fd9996/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java#L71
I tried using `@SuppressWarnings()` for the above line, but that also doesn't help since it's a parsing error for PMD.
Thoughts?
--
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] Remove support for Java 8 (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846700300
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
error was in `druid-thrift-extensions`:
```
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-dependency-plugin:3.2.0:go-offline (default-cli) on project druid-thrift-extensions: org.eclipse.aether.resolution.DependencyResolutionException: Failed to collect dependencies at com.twitter.elephantbird:elephant-bird-core:jar:4.17 -> com.hadoop.gplcompression:hadoop-lzo:jar:0.4.19: Failed to read artifact descriptor for com.hadoop.gplcompression:hadoop-lzo:jar:0.4.19: Could not transfer artifact com.hadoop.gplcompression:hadoop-lzo:pom:0.4.19 from/to maven-default-http-blocker (http://0.0.0.0/): Blocked mirror for repositories: [twitter (http://maven.twttr.com, default, releases+snapshots)] -> [Help 1]
```
* this seems to be deliberately excluded from the pom [here](https://github.com/apache/druid/blob/24a1fafaa745fb93bf1522490c095cd689fd9996/extensions-contrib/thrift-extensions/pom.xml#L83-L86)
* and its documented to download it separately [here](https://github.com/apache/druid/blob/master/docs/development/extensions-contrib/thrift.md#lzo-support)
* at the same these artifacts are 10+ year old [ones](https://mvnrepository.com/artifact/com.hadoop.gplcompression)...I don't think we want them in the base build
I'm not sure why `go-offline` still want to read that excluded artifact...and how it did that before...
but instead of adding this repo to the main pom; can we only add it for the `extensions-contrib/thrift-extensions` module?
--
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] Deprecate Java 8 (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1843421421
##########
.github/workflows/cron-job-its.yml:
##########
@@ -60,8 +61,8 @@ jobs:
uses: ./.github/workflows/reusable-standard-its.yml
needs: build
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 21.0.4
Review Comment:
This should be changed. These experiments can be done on private branches and not on the main branch.
Build jdk <=runtimeJdk.
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846554440
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
@kgyrtkirk Without addition of this mirror, we get the following error:
```
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-dependency-plugin:3.1.2:go-offline (default-cli) on project druid-thrift-extensions: org.eclipse.aether.resolution.DependencyResolutionException: Failed to collect dependencies at com.twitter.elephantbird:elephant-bird-core:jar:4.17 -> com.hadoop.gplcompression:hadoop-lzo:jar:0.4.19: Failed to read artifact descriptor for com.hadoop.gplcompression:hadoop-lzo:jar:0.4.19: The following artifacts could not be resolved: com.hadoop.gplcompression:hadoop-lzo:pom:0.4.19 (present, but unavailable): Could not transfer artifact com.hadoop.gplcompression:hadoop-lzo:pom:0.4.19 from/to central.maven.org (https://central.maven.org/maven2): central.maven.org: nodename nor servname provided, or not known: Unknown host central.maven.org: nodename nor servname provided, or not known -> [Help 1]
```
Sample CI failure: https://github.com/apache/druid/actions/runs/11777967965/job/32803425792
I'm also able to get the error locally using the same command that the above check uses:
```
mvn -q clean install dependency:go-offline -P dist -P skip-static-checks,skip-tests -Dmaven.javadoc.skip=true -Dcyclonedx.skip=true
```
--
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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841921159
##########
.github/workflows/revised-its.yml:
##########
@@ -86,8 +86,8 @@ jobs:
uses: ./.github/workflows/reusable-revised-its.yml
if: ${{ needs.changes.outputs.core == 'true' || needs.changes.outputs.common-extensions == 'true' }}
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 11
+ runtime_jdk: 17
Review Comment:
(Like, Druid built with Java 8 running in Java 11 environment)
--
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] Deprecate Java 8 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841991629
##########
.github/workflows/codeql.yml:
##########
@@ -40,7 +40,7 @@ jobs:
- uses: actions/setup-java@v4
with:
distribution: 'zulu'
- java-version: '8'
+ java-version: '11'
Review Comment:
i think we should strive to default to the most recent lts version we support, we should be deprecating java 11 support soon too tbh, its getting quite old too
if we're worried about compiled with java 17 and running on 11 we could add make one of the mixed version things configured in this manner?
--
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] Remove support for Java 8 (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #17466:
URL: https://github.com/apache/druid/pull/17466
--
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] Remove support for Java 8 (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1843476669
##########
benchmarks/pom.xml:
##########
@@ -239,7 +240,7 @@
<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<jmh.version>1.21</jmh.version>
- <javac.target>1.8</javac.target>
+ <javac.target>11</javac.target>
Review Comment:
why specify this?
```
git grep javac.target
```
returns only this line...
isn't this unused?
##########
extensions-contrib/compressed-bigdecimal/pom.xml:
##########
@@ -145,7 +145,7 @@
<dependency>
<groupId>com.google.inject</groupId>
<artifactId>guice</artifactId>
- <version>4.1.0</version>
+ <version>4.2.2</version>
Review Comment:
remove the version; let `dependencyManagement` handle it
##########
pom.xml:
##########
@@ -70,9 +70,9 @@
</scm>
<properties>
- <maven.compiler.source>1.8</maven.compiler.source>
- <maven.compiler.target>1.8</maven.compiler.target>
- <java.version>8</java.version>
+ <java.version>11</java.version>
+ <maven.compiler.source>${java.version}</maven.compiler.source>
+ <maven.compiler.target>${java.version}</maven.compiler.target>
Review Comment:
the stuff in profile `java-9+` should be applied by default
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
do we need to specify this version?
why can't we let the parent pom define it?
https://mvnrepository.com/artifact/org.apache/apache/25 is also pretty old; but it still sets 3.2.0
##########
integration-tests-ex/image/docker/Dockerfile:
##########
@@ -28,7 +28,7 @@
# This Dockerfile prefers to use the COPY command over ADD.
# See: https://phoenixnap.com/kb/docker-add-vs-copy
-ARG JDK_VERSION=8-slim-buster
+ARG JDK_VERSION=17-slim-buster
Review Comment:
note: `buster` (debian10) is a pretty old release ; this image was not updated for [3 years](https://hub.docker.com/_/openjdk/tags?name=17-slim-buster)
we should probably upgrade to a more recent version`bookworm` (or `bullseye`)
##########
.github/workflows/cron-job-its.yml:
##########
@@ -60,8 +61,8 @@ jobs:
uses: ./.github/workflows/reusable-standard-its.yml
needs: build
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 21.0.4
+ runtime_jdk: 17
Review Comment:
note: I've looked it up - and the `--release` option have changed a lot of things :)
api compatibility is guaranteed :)
https://maven.apache.org/plugins/maven-compiler-plugin/examples/set-compiler-release.html#setting-the-release-of-the-java-compiler
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
I don't fully understand this....what causes this to be needed;
I've just built a few modules w/o this repo after cleaning `.m2/repository` ; is this really needed?
can we upgrade something insterad? why do we even use `maven-dependency-plugin:3.1.2` - that also seem to date back to [2020](https://mvnrepository.com/artifact/org.apache.maven.plugins/maven-dependency-plugin/3.1.2)...
##########
pom.xml:
##########
@@ -96,7 +96,8 @@
<errorprone.version>2.35.1</errorprone.version>
<fastutil.version>8.5.4</fastutil.version>
<guava.version>32.0.1-jre</guava.version>
- <guice.version>4.1.0</guice.version>
+ <!-- Need Guice 4.2.2 for Java 11 support: https://github.com/google/guice/wiki/Guice510 -->
Review Comment:
remove this comment; it has no value after merging the PR; you could upgrade `guice` separately in a PR and say that its needed because of jdk11 in the PR description; then the commit hash of this line will link to that conversation and if someone is really interested could get to it....
##########
pom.xml:
##########
@@ -70,9 +70,9 @@
</scm>
<properties>
- <maven.compiler.source>1.8</maven.compiler.source>
- <maven.compiler.target>1.8</maven.compiler.target>
- <java.version>8</java.version>
+ <java.version>11</java.version>
+ <maven.compiler.source>${java.version}</maven.compiler.source>
Review Comment:
remove: `maven.compiler.target` and `maven.compiler.source` only specify the `release` and forget the others
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1847713652
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
I explored further on this, have the following possible options:
### Option 1
- Remove version from maven-dependency-plugin, hence it defaults to 3.2.0.
- Set version of maven-pmd-plugin as 3.26.0 (this is the latest version).
- Remove `<rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>` from [pmd-ruleset.xml](https://github.com/apache/druid/blob/master/codestyle/pmd-ruleset.xml) (because there were hundreds of errors)
- With the above changes, we get a LOT of warnings (hence the check fails) like:
```
[WARNING] Non-test scoped test only dependencies found:
[WARNING] org.apache.zookeeper:zookeeper-jute:jar:3.8.4:compile
[WARNING] commons-codec:commons-codec:jar:1.16.1:compile
[WARNING] joda-time:joda-time:jar:2.12.5:compile
[WARNING] org.apache.curator:curator-client:jar:5.5.0:compile
```
To address the above, I added `<scope>provided</scope>` to the dependencies mentioned in the above warning message. I did that in 5 modules, but since this would have to be done manually for (most likely) almost all 80 modules, I thought we should discuss first if we want to go with this approach.
### Option 2
- Set maven-dependency-plugin version to 3.3.0.
- This allows us to add a configuration to pom.xml to ignore these type of errors, so add the following:
```
<configuration>
<ignoredNonTestScopedDependencies>
<ignoredNonTestScopedDependency>*</ignoredNonTestScopedDependency>
</ignoredNonTestScopedDependencies>
</configuration>
```
- Set version of maven-pmd-plugin as 3.26.0 (this is the latest version).
- Remove `<rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>` from [pmd-ruleset.xml](https://github.com/apache/druid/blob/master/codestyle/pmd-ruleset.xml) (because there were hundreds of errors)
### Option 3
- Keep the existing PR as it is. This means:
- maven-dependency-plugin == 3.1.2
- maven-pmd-plugin == 3.16.0
- Once this PR is merged, have a follow-up work item, where we proceed with either option 1 or option 2. In that follow-up PR, we can also work on the `UnnecessaryFullyQualifiedName` violations, instead of removing this from `pmd-ruleset.xml`
<hr>
My thoughts: Option 1 would affect a lot of files, and make it difficult to review the current PR. So I feel that we should either go with Option 2 in this PR, or go with Option 3.
What do you think? Would appreciate your inputs on the above, thanks!
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1847736567
##########
pom.xml:
##########
@@ -70,9 +70,9 @@
</scm>
<properties>
- <maven.compiler.source>1.8</maven.compiler.source>
- <maven.compiler.target>1.8</maven.compiler.target>
- <java.version>8</java.version>
+ <java.version>11</java.version>
+ <maven.compiler.source>${java.version}</maven.compiler.source>
+ <maven.compiler.target>${java.version}</maven.compiler.target>
Review Comment:
Makes sense, have removed the java-9+ profile and moved those properties to default section. Thanks for pointing this out!
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on PR #17466:
URL: https://github.com/apache/druid/pull/17466#issuecomment-2489983522
@clintropolis Makes sense, have made the change.
--
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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841918736
##########
.github/workflows/revised-its.yml:
##########
@@ -86,8 +86,8 @@ jobs:
uses: ./.github/workflows/reusable-revised-its.yml
if: ${{ needs.changes.outputs.core == 'true' || needs.changes.outputs.common-extensions == 'true' }}
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 11
+ runtime_jdk: 17
Review Comment:
I'm not sure, but some kind of backward compatibility was my guess. Have tried to follow the same pattern as it existed originally.
--
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] Deprecate Java 8 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841977047
##########
.github/workflows/revised-its.yml:
##########
@@ -86,8 +86,8 @@ jobs:
uses: ./.github/workflows/reusable-revised-its.yml
if: ${{ needs.changes.outputs.core == 'true' || needs.changes.outputs.common-extensions == 'true' }}
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 11
+ runtime_jdk: 17
Review Comment:
iirc this was originally done after we first added support for java versions greater than 8 to make sure stuff build in different versions worked as expected, i don't think we need mixed images like this anymore
--
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] Deprecate Java 8 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841977047
##########
.github/workflows/revised-its.yml:
##########
@@ -86,8 +86,8 @@ jobs:
uses: ./.github/workflows/reusable-revised-its.yml
if: ${{ needs.changes.outputs.core == 'true' || needs.changes.outputs.common-extensions == 'true' }}
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 11
+ runtime_jdk: 17
Review Comment:
iirc this was originally done after we first added support for java versions greater than 8, i don't think we need mixed images like this anymore
--
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] Deprecate Java 8 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841979390
##########
.github/workflows/codeql.yml:
##########
@@ -40,7 +40,7 @@ jobs:
- uses: actions/setup-java@v4
with:
distribution: 'zulu'
- java-version: '8'
+ java-version: '11'
Review Comment:
why 11 by default instead of 17? I was hoping we'd have 21 officially supported by now but I'm not sure it is yet which would be my real preference to have as the default since its the latest lts
--
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] Remove support for Java 8 (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846700300
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
error was in `druid-thrift-extensions`:
```
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-dependency-plugin:3.2.0:go-offline (default-cli) on project druid-thrift-extensions: org.eclipse.aether.resolution.DependencyResolutionException: Failed to collect dependencies at com.twitter.elephantbird:elephant-bird-core:jar:4.17 -> com.hadoop.gplcompression:hadoop-lzo:jar:0.4.19: Failed to read artifact descriptor for com.hadoop.gplcompression:hadoop-lzo:jar:0.4.19: Could not transfer artifact com.hadoop.gplcompression:hadoop-lzo:pom:0.4.19 from/to maven-default-http-blocker (http://0.0.0.0/): Blocked mirror for repositories: [twitter (http://maven.twttr.com, default, releases+snapshots)] -> [Help 1]
```
* this seems to be deliberately excluded from the pom [here](https://github.com/apache/druid/blob/24a1fafaa745fb93bf1522490c095cd689fd9996/extensions-contrib/thrift-extensions/pom.xml#L83-L86)
* and its documented to download it separately [here](https://github.com/apache/druid/blob/master/docs/development/extensions-contrib/thrift.md#lzo-support)
* at the same these artifacts are 10+ year old [ones](https://mvnrepository.com/artifact/com.hadoop.gplcompression)...might have been newer ones..replacements...not sure why they weren't updated
I'm also a bit puzzled why `go-offline` still want to read that excluded artifact...
but instead of adding this repo to the main pom; can we only add it for the `extensions-contrib/thrift-extensions` module?
--
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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841897916
##########
server/src/main/java/org/apache/druid/curator/discovery/CuratorDruidLeaderSelector.java:
##########
@@ -58,7 +59,7 @@ public class CuratorDruidLeaderSelector implements DruidLeaderSelector
private final AtomicReference<LeaderLatch> leaderLatch = new AtomicReference<>();
private volatile boolean leader = false;
- private volatile int term = 0;
+ private final AtomicInteger term = new AtomicInteger(0);
Review Comment:
It's a spotbugs pattern name called `VO_VOLATILE_INCREMENT`: https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#vo-an-increment-to-a-volatile-field-isn-t-atomic-vo-volatile-increment
--
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] Remove support for Java 8 (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846746947
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
I'm not sure if this feature is even being used:
`elephant-bird` didn't had any releases for quite a few years now...
on the github page no real person opened a PR in the last 2 years or so ....last reported issues is also 2 years old..
I doubt its being used in production - what do you think about removing 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: 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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1848563017
##########
check_test_suite.py:
##########
@@ -22,7 +22,7 @@
# this script does some primitive examination of git diff to determine if a test suite needs to be run or not
# these jobs should always be run, no matter what
-always_run_jobs = ['license checks', '(openjdk8) packaging check', '(openjdk11) packaging check']
+always_run_jobs = ['license checks', '(openjdk11) packaging check']
Review Comment:
Closing this comment as it's not relevant anymore with the latest changes of the 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: 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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841912390
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/CursorFactoryRowsAndColumns.java:
##########
@@ -112,12 +112,8 @@ private static RowsAndColumns materialize(CursorFactory cursorFactory)
cursor.advance();
}
- if (writer == null) {
- return new EmptyRowsAndColumns();
- } else {
- final byte[] bytes = writer.toByteArray();
- return new ColumnBasedFrameRowsAndColumns(Frame.wrap(bytes), rowSignature);
- }
+ final byte[] bytes = writer.toByteArray();
Review Comment:
It's a spotbugs pattern named `RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE`: https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#rcn-redundant-nullcheck-of-value-known-to-be-null-rcn-redundant-nullcheck-of-null-value
I'm not sure why it wasn't showing up in Java 8. But the change seems minor, and is legit, so I ended up making it.
We also have other places in the code where we expect `frameWriterFactory.newFrameWriter()` to return non-null. Example: https://github.com/apache/druid/blob/75d9ece665460fdfab119576bae707ba7f60bbcd/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/common/OffsetLimitFrameProcessor.java#L149
--
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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841987300
##########
.github/workflows/codeql.yml:
##########
@@ -40,7 +40,7 @@ jobs:
- uses: actions/setup-java@v4
with:
distribution: 'zulu'
- java-version: '8'
+ java-version: '11'
Review Comment:
> why 11 by default instead of 17?
I thought we wanted to move to the immediate next version, instead of skipping versions. Also, was discussing this with Karan, and we weren't sure of how Druid built with 17 might behave on Java 11 environment.
--
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] Remove support for Java 8 (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846671730
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
I've removed the version setting for the pmd plugin and it started working; it found a few new warnings - but we could possibly disable the checks for those for now?
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1848559786
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
> note: I'm not sure why - but somehow I don't see Non-test scoped test only dependencies found issue right now locally
@kgyrtkirk It's because we get those issues on updating `maven-pmd-plugin` to the latest version `3.26.0`
--
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] Remove support for Java 8 (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1848549464
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
note: I'm not sure why - but somehow I don't see `Non-test scoped test only dependencies found` issue right now locally- maybe I'm using some different version of maven or something; or something have affected that in the PR?
anyway: Option 2 sounds like a nice way of mitigating that...I always feeled that its pointless to declare these for tests as well...
--
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] Remove support for Java 8 (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1848553567
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
lets leave it then for now...
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846757283
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
> I doubt its being used in production - what do you think about removing it?
Removing what exactly? "LZO support" from the thrift extension? 😅
I don't have much context on this part, so can't comment. Regardless, this would require a discussion with the wider team in my opinion.
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846711428
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
> but instead of adding this repo to the main pom; can we only add it for the extensions-contrib/thrift-extensions module?
I tried that originally. If you look at these 2 commits:
<img width="886" alt="image" src="https://github.com/user-attachments/assets/8976d3af-d6ff-42de-bcbb-02472bc6565d">
[First commit](https://github.com/apache/druid/pull/17466/commits/ea17aa1c463d4cc6e0b6a6ed4b2ba92726251ca7) added it to the `thrift-extensions` module, but that still caused the same error: [CI run for that commit](https://github.com/apache/druid/actions/runs/11782156018/job/32816565859)
[Second commit](https://github.com/apache/druid/pull/17466/commits/6d127c1e860c0f6751efb18621eff1d938dce087) moved it to the parent pom, which fixed the issue.
Thoughts?
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1849722574
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
> anyway: Option 2 sounds like a nice way of mitigating that...I always feeled that its pointless to declare these for tests as well...
Have pushed the change to the PR for option 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: 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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841896067
##########
.github/workflows/unit-and-integration-tests-unified.yml:
##########
@@ -162,7 +162,7 @@ jobs:
fail-fast: false
matrix:
# Use JDK 21.0.4 to work around https://github.com/apache/druid/issues/17429
- jdk: [ '11', '17', '21.0.4' ]
+ jdk: [ '17', '21.0.4' ]
Review Comment:
Oops, nope. Thanks for pointing this out!
Have added it back!
--
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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841901631
##########
check_test_suite.py:
##########
@@ -22,7 +22,7 @@
# this script does some primitive examination of git diff to determine if a test suite needs to be run or not
# these jobs should always be run, no matter what
-always_run_jobs = ['license checks', '(openjdk8) packaging check', '(openjdk11) packaging check']
+always_run_jobs = ['license checks', '(openjdk11) packaging check']
Review Comment:
I'm not sure on this either. But I'd prefer to not mix in Java 17 stuff in this patch - I think it should be a separate work item to go through all CI checks from Java 17 point of view, and decide what we want enabled. Hope that works!
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1847096840
##########
integration-tests-ex/image/docker/Dockerfile:
##########
@@ -28,7 +28,7 @@
# This Dockerfile prefers to use the COPY command over ADD.
# See: https://phoenixnap.com/kb/docker-add-vs-copy
-ARG JDK_VERSION=8-slim-buster
+ARG JDK_VERSION=17-slim-buster
Review Comment:
I don't see a recent image of `bookworm` or `bullseye` for JDK 17 - they also seem to be 3 years older.
I see recent images like [openjdk:24-ea-17-jdk-slim-bullseye](https://hub.docker.com/layers/library/openjdk/24-ea-17-jdk-slim-bullseye/images/sha256-934218e043ebb38a8dee819530aca96474d69d819306a5370555675c74869224?context=explore) and [openjdk:24-ea-17-jdk-slim-bookworm](https://hub.docker.com/layers/library/openjdk/24-ea-17-jdk-slim-bookworm/images/sha256-7fe4b436108ba9761c6b64e48867c4bb953562a42cfcd546993ce1e5f0771970?context=explore), but I don't think that's what we need? (IIUC, these names mean that 17 is the build number for Java 24's EA release?)
Can you suggest which image you had in mind? Please let me know if I'm missing something, thanks!
--
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] Deprecate Java 8 (druid)
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841653525
##########
.github/workflows/revised-its.yml:
##########
@@ -86,8 +86,8 @@ jobs:
uses: ./.github/workflows/reusable-revised-its.yml
if: ${{ needs.changes.outputs.core == 'true' || needs.changes.outputs.common-extensions == 'true' }}
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 11
+ runtime_jdk: 17
Review Comment:
Just curious, why is the runtime higher?
##########
check_test_suite.py:
##########
@@ -22,7 +22,7 @@
# this script does some primitive examination of git diff to determine if a test suite needs to be run or not
# these jobs should always be run, no matter what
-always_run_jobs = ['license checks', '(openjdk8) packaging check', '(openjdk11) packaging check']
+always_run_jobs = ['license checks', '(openjdk11) packaging check']
Review Comment:
I'm not familiar with this, but should we start running the check for 17 now?
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/CursorFactoryRowsAndColumns.java:
##########
@@ -112,12 +112,8 @@ private static RowsAndColumns materialize(CursorFactory cursorFactory)
cursor.advance();
}
- if (writer == null) {
- return new EmptyRowsAndColumns();
- } else {
- final byte[] bytes = writer.toByteArray();
- return new ColumnBasedFrameRowsAndColumns(Frame.wrap(bytes), rowSignature);
- }
+ final byte[] bytes = writer.toByteArray();
Review Comment:
Removing a null check for whatever reason seems weird. Is there a reason that it wasn't showing up in Java 8? If it was suppressed there, perhaps it should be here as well.
##########
server/src/main/java/org/apache/druid/curator/discovery/CuratorDruidLeaderSelector.java:
##########
@@ -58,7 +59,7 @@ public class CuratorDruidLeaderSelector implements DruidLeaderSelector
private final AtomicReference<LeaderLatch> leaderLatch = new AtomicReference<>();
private volatile boolean leader = false;
- private volatile int term = 0;
+ private final AtomicInteger term = new AtomicInteger(0);
Review Comment:
Just curious, what is the checkstyle rule that was failing here?
##########
.github/workflows/unit-and-integration-tests-unified.yml:
##########
@@ -162,7 +162,7 @@ jobs:
fail-fast: false
matrix:
# Use JDK 21.0.4 to work around https://github.com/apache/druid/issues/17429
- jdk: [ '11', '17', '21.0.4' ]
+ jdk: [ '17', '21.0.4' ]
Review Comment:
Does 11 need to be removed here?
--
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] Remove support for Java 8 (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #17466:
URL: https://github.com/apache/druid/pull/17466#issuecomment-2489822305
it looks like java 17 is still running in the second phase of unit tests, these can be removed i think since they replaced the java 8 tests in phase 1
--
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] Remove support for Java 8 (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1846711908
##########
pom.xml:
##########
@@ -271,6 +272,16 @@
<enabled>false</enabled>
</snapshots>
</repository>
+
+ <!--
+ maven-dependency-plugin:3.1.2 seems to have updated HTTP repository access behavior.
Review Comment:
I feel like we could place `elephantbird` under a profile - and the [single usage](https://github.com/apache/druid/blob/24a1fafaa745fb93bf1522490c095cd689fd9996/extensions-contrib/thrift-extensions/src/main/java/org/apache/druid/data/input/thrift/ThriftInputRowParser.java#L135-L137) of `ThriftWritable` could be done via reflection and forget all that repo/etc
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1847097891
##########
.github/workflows/cron-job-its.yml:
##########
@@ -60,8 +61,8 @@ jobs:
uses: ./.github/workflows/reusable-standard-its.yml
needs: build
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 21.0.4
Review Comment:
Have made the change.
--
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] Remove support for Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1847713652
##########
pom.xml:
##########
@@ -1830,7 +1851,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
- <version>3.1.1</version>
+ <version>3.1.2</version>
Review Comment:
I explored further on this, have the following possible options:
### Option 1
- Remove version from maven-dependency-plugin, hence it defaults to 3.2.0.
- Set version of maven-pmd-plugin as 3.26.0 (this is the latest version).
- Remove `<rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>` from [pmd-ruleset.xml](https://github.com/apache/druid/blob/master/codestyle/pmd-ruleset.xml) (because there were hundreds of errors)
- With the above changes, we get a LOT of warnings (hence the check fails) like:
```
[WARNING] Non-test scoped test only dependencies found:
[WARNING] org.apache.zookeeper:zookeeper-jute:jar:3.8.4:compile
[WARNING] commons-codec:commons-codec:jar:1.16.1:compile
[WARNING] joda-time:joda-time:jar:2.12.5:compile
[WARNING] org.apache.curator:curator-client:jar:5.5.0:compile
```
To address the above, I added `<scope>provided</scope>` to the dependencies mentioned in the above warning message. I did that in 5 modules, but since this would have to be done manually for (most likely) almost all 80 modules, I thought we should discuss first if we want to go with this approach.
### Option 2
- Set maven-dependency-plugin version to 3.3.0. This allows us to add a configuration to pom.xml to ignore these type of errors, so add the following:
```
<configuration>
<ignoredNonTestScopedDependencies>
<ignoredNonTestScopedDependency>*</ignoredNonTestScopedDependency>
</ignoredNonTestScopedDependencies>
</configuration>
```
- Set version of maven-pmd-plugin as 3.26.0 (this is the latest version).
- Remove `<rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>` from [pmd-ruleset.xml](https://github.com/apache/druid/blob/master/codestyle/pmd-ruleset.xml) (because there were hundreds of errors)
### Option 3
- Keep the existing PR as it is. This means:
- maven-dependency-plugin == 3.1.2
- maven-pmd-plugin == 3.16.0
- Once this PR is merged, have a follow-up work item, where we proceed with either option 1 or option 2. In that follow-up PR, we can also work on the `UnnecessaryFullyQualifiedName` violations, instead of removing this from `pmd-ruleset.xml`
<hr>
My thoughts: Option 1 would affect a lot of files, and make it difficult to review the current PR. So I feel that we should either go with Option 2 in this PR, or go with Option 3.
What do you think? Would appreciate your inputs on the above, thanks!
--
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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1843326795
##########
.github/workflows/codeql.yml:
##########
@@ -40,7 +40,7 @@ jobs:
- uses: actions/setup-java@v4
with:
distribution: 'zulu'
- java-version: '8'
+ java-version: '11'
Review Comment:
@clintropolis Thanks for the inputs!
I've updated the PR as per the offline discussion. Can you please take a look again? Thanks!
--
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] Deprecate Java 8 (druid)
Posted by "Akshat-Jain (via GitHub)" <gi...@apache.org>.
Akshat-Jain commented on code in PR #17466:
URL: https://github.com/apache/druid/pull/17466#discussion_r1841989606
##########
.github/workflows/revised-its.yml:
##########
@@ -86,8 +86,8 @@ jobs:
uses: ./.github/workflows/reusable-revised-its.yml
if: ${{ needs.changes.outputs.core == 'true' || needs.changes.outputs.common-extensions == 'true' }}
with:
- build_jdk: 8
- runtime_jdk: 11
+ build_jdk: 11
+ runtime_jdk: 17
Review Comment:
> i don't think we need mixed images like this anymore
Why though? I mean, "to make sure stuff build in different versions worked as expected" is still a requirement, so we should continue testing 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: 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