gemini-code-assist[bot] commented on code in PR #38324:
URL: https://github.com/apache/beam/pull/38324#discussion_r3162541050
##########
runners/spark/job-server/spark_job_server.gradle:
##########
@@ -28,7 +28,10 @@ apply plugin: 'application'
// we need to set mainClassName before applying shadow plugin
mainClassName = "org.apache.beam.runners.spark.SparkJobServerDriver"
+def parentSparkVersion = project.parent.findProperty('spark_version') ?: ''
+
applyJavaNature(
+ requireJavaVersion: (parentSparkVersion.startsWith("4") ?
org.gradle.api.JavaVersion.VERSION_17 : null),
Review Comment:
Using `project.parent.findProperty` is fragile as it will throw a
`NullPointerException` if the project has no parent (e.g., if it's the root
project). It is more idiomatic and safer to use
`project.findProperty('spark_version')`, which searches the project and all its
ancestors. Additionally, using the variable name `sparkVersion` is more
standard as properties are inherited across the project hierarchy.
```
def sparkVersion = project.findProperty('spark_version') ?: ''
applyJavaNature(
requireJavaVersion: (sparkVersion.startsWith("4") ?
org.gradle.api.JavaVersion.VERSION_17 : null),
```
##########
runners/spark/spark_runner.gradle:
##########
@@ -21,6 +21,7 @@ import groovy.json.JsonOutput
apply plugin: 'org.apache.beam.module'
applyJavaNature(
enableStrictDependencies: true,
+ requireJavaVersion: (spark_version.startsWith("4") ?
org.gradle.api.JavaVersion.VERSION_17 : null),
Review Comment:
This check uses `startsWith("4")`, which is less robust than the
`isSparkAtLeast` helper introduced later in this file. To improve consistency
and robustness, consider moving the `isSparkAtLeast` definition to the top of
the script and using it here (e.g., `isSparkAtLeast("4.0.0")`).
##########
runners/spark/spark_runner.gradle:
##########
@@ -35,6 +36,16 @@ applyJavaNature(
description = "Apache Beam :: Runners :: Spark $spark_version"
+// Numeric version comparison (lexicographic string compare was fragile — e.g.
"3.10.0" < "3.5.0").
+def isSparkAtLeast = { String minVersion ->
+ def parts = spark_version.tokenize('.-').findAll { it.isInteger()
}*.toInteger()
+ def minParts = minVersion.tokenize('.')*.toInteger()
Review Comment:
The parsing of `minVersion` should be as robust as the parsing of
`spark_version` at line 41. Using `tokenize('.-').findAll { it.isInteger() }`
ensures that any non-numeric parts in the version string (e.g., if a developer
passes a version with a suffix like `-preview`) do not cause a
`NumberFormatException` during the `toInteger()` conversion.
```
def minParts = minVersion.tokenize('.-').findAll { it.isInteger()
}*.toInteger()
```
--
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]
