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-unsubscr...@druid.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org

Reply via email to