gemini-code-assist[bot] commented on code in PR #39242:
URL: https://github.com/apache/beam/pull/39242#discussion_r3544750332
##########
runners/spark/job-server/spark_job_server.gradle:
##########
@@ -267,13 +267,28 @@ tasks.register("validatesRunnerSickbay", Test) {
def jobPort = BeamModulePlugin.getRandomPort()
def artifactPort = BeamModulePlugin.getRandomPort()
+def sparkJobServerJvmArgs() {
+ def testJavaVer = project.findProperty('testJavaVersion') ?
(project.property('testJavaVersion') as int) :
JavaVersion.current().majorVersion.toInteger()
Review Comment:
In Groovy/Gradle, using `as int` on a property that might be a String can
throw a `NumberFormatException` if the property is not a valid integer.
Additionally, `JavaVersion.current().majorVersion` is already a String, so
calling `.toInteger()` on it is safe, but we should make sure `testJavaVersion`
is safely parsed.
Consider using `Integer.parseInt()` or a safe cast with a fallback to avoid
potential build failures if `testJavaVersion` is configured incorrectly.
```
def testJavaVer = project.findProperty('testJavaVersion') ?
(project.property('testJavaVersion').toString().toInteger()) :
JavaVersion.current().majorVersion.toInteger()
```
##########
sdks/python/scripts/run_job_server.sh:
##########
@@ -107,7 +113,7 @@ case $STARTSTOP in
fi
echo "Launching job server @ $JOB_PORT ..."
- "$JAVA_CMD" -jar $JOB_SERVER_JAR --job-port=$JOB_PORT
--artifact-port=$ARTIFACT_PORT --expansion-port=0 $ADDITIONAL_ARGS
>$TEMP_DIR/$FILE_BASE.log 2>&1 </dev/null &
+ "$JAVA_CMD" $JVM_ARGS -jar $JOB_SERVER_JAR --job-port=$JOB_PORT
--artifact-port=$ARTIFACT_PORT --expansion-port=0 $ADDITIONAL_ARGS
>$TEMP_DIR/$FILE_BASE.log 2>&1 </dev/null &
Review Comment:
When `$JVM_ARGS` is unquoted, it undergoes word splitting, which is
necessary here to pass multiple JVM arguments (like `--add-opens`) as separate
parameters to the `java` command. However, if `$JVM_ARGS` is empty or unset,
this can sometimes lead to unexpected behavior depending on shell settings, or
if any of the arguments contain spaces.
To ensure robustness and avoid issues with empty values, it is safer to use
an array for arguments in bash, or explicitly handle the expansion. Since this
script is executed via `sh` (which might be `dash` or `bash` depending on the
system), we should be careful. If we are sure it's run with `bash` (the shebang
of `run_job_server.sh` is likely `#!/bin/bash`), using a bash array is the most
robust way to handle list of arguments with potential spaces. If it must remain
a string, we should ensure `JVM_ARGS` is initialized to an empty string at the
top of the script to prevent unbound variable errors if `set -u` 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]
