Sxnan commented on code in PR #356:
URL: https://github.com/apache/flink-agents/pull/356#discussion_r2625440571


##########
api/pom.xml:
##########
@@ -65,4 +65,53 @@ under the License.
         </dependency>
     </dependencies>
 
+    <profiles>
+        <profile>
+            <id>mcp-enabled</id>

Review Comment:
   It is better to name the profile as `java-17`, as it is activated 
automatically when the JDK version is 17+.
   
   Same for the other poms



##########
api/pom.xml:
##########


Review Comment:
   We should use the classifier for the java17 jar, such as `java-17`. 
   
   So that after releasing, user can depend on the java17 jar by specifying the 
classifier in their pom, for example:
   
   ```
   <dependency>
       <groupId>org.apache.flink</groupId>
       <artifactId>flink-agents-api</artifactId>
       <version>0.2.0</version>
       <classifier>java-17</classifier>
   </dependency>
   ```



##########
pom.xml:
##########
@@ -95,7 +122,7 @@ under the License.
                 </property>
             </activation>
             <properties>
-                <target.java.version>11</target.java.version>
+                <target.java.version>17</target.java.version>

Review Comment:
   I think we should target for jdk11 by default for better compatibility.



##########
pom.xml:
##########
@@ -87,6 +88,32 @@ under the License.
     </dependencies>
 
     <profiles>
+        <profile>
+            <id>java-11</id>
+            <activation>
+                <jdk>[11,17)</jdk>
+            </activation>
+            <properties>
+                <target.java.version>11</target.java.version>
+                <maven.compiler.source>11</maven.compiler.source>
+                <maven.compiler.target>11</maven.compiler.target>
+                <mcp.enabled>false</mcp.enabled>
+            </properties>
+        </profile>
+
+        <profile>
+            <id>java-17-plus</id>

Review Comment:
   I think `java-17` should be enough. Otherwise, when we add the profile for 
java21, we need to update the id of this profile as well. 



##########
pom.xml:
##########
@@ -53,6 +53,7 @@ under the License.
         <mockito.version>5.8.0</mockito.version>
         <gpg.useagent>true</gpg.useagent>
         <arguments />
+        <mcp.enabled>true</mcp.enabled>

Review Comment:
   I cannot find where it is used.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to