gemini-code-assist[bot] commented on code in PR #39303:
URL: https://github.com/apache/beam/pull/39303#discussion_r3570475557


##########
runners/flink/job-server/flink_job_server.gradle:
##########
@@ -239,6 +239,27 @@ tasks.register("validatesPortableRunner") {
   dependsOn validatesPortableRunnerStreamingCheckpoint
 }
 
+def flinkJobServerJvmArgs() {
+  def testJavaVer = project.findProperty('testJavaVersion') ? 
(project.property('testJavaVersion') as int) : 
JavaVersion.current().majorVersion.toInteger()
+  if (testJavaVer >= 17) {

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   Instead of manually parsing the Java version string to an integer (which can 
be fragile depending on the format of `testJavaVersion`), you can use Gradle's 
built-in `JavaVersion.toVersion()` helper. This is much more robust and 
idiomatic.
   
   ```
     def javaVersion = 
JavaVersion.toVersion(project.findProperty('testJavaVersion') ?: 
JavaVersion.current())
     if (javaVersion.isJava17Compatible()) {
   ```



##########
runners/flink/job-server/flink_job_server.gradle:
##########
@@ -239,6 +239,27 @@ tasks.register("validatesPortableRunner") {
   dependsOn validatesPortableRunnerStreamingCheckpoint
 }
 
+def flinkJobServerJvmArgs() {
+  def testJavaVer = project.findProperty('testJavaVersion') ? 
(project.property('testJavaVersion') as int) : 
JavaVersion.current().majorVersion.toInteger()
+  if (testJavaVer >= 17) {
+    return [
+      "--add-opens=java.base/sun.nio.ch=ALL-UNNAMED",
+      "--add-opens=java.base/java.nio=ALL-UNNAMED",
+      "--add-opens=java.base/java.util=ALL-UNNAMED",
+      "--add-opens=java.base/java.lang.invoke=ALL-UNNAMED",
+      "--add-opens=java.base/java.lang=ALL-UNNAMED",
+      "-Djava.security.manager=allow",
+    ]
+  }
+  return []
+}
+
+[validatesPortableRunnerDocker, validatesPortableRunnerBatchDataSet, 
validatesPortableRunnerBatch, validatesPortableRunnerStreaming, 
validatesPortableRunnerStreamingCheckpoint].each { task ->
+  task.configure {
+    jvmArgs += flinkJobServerJvmArgs()

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   Using `jvmArgs(...)` is the standard and safer Gradle DSL method to append 
JVM arguments to a task, rather than using the `+=` operator which can fail if 
`jvmArgs` is not pre-initialized.
   
   ```
       jvmArgs(flinkJobServerJvmArgs())
   ```



-- 
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