minor: [JDK upgrade] Update forking task runner to use bin/run-java#19709
Conversation
| |`druid.indexer.runner.allowedPrefixes`|Whitelist of prefixes for configs that can be passed down to child peons.|`com.metamx`, `druid`, `org.apache.druid`, `user.timezone`, `file.encoding`, `java.io.tmpdir`, `hadoop`| | ||
| |`druid.indexer.runner.classpath`|Java classpath for the peon.|`System.getProperty("java.class.path")`| | ||
| |`druid.indexer.runner.javaCommand`|Command required to execute java.|java| | ||
| |`druid.indexer.runner.javaCommand`| Command required to execute java. When left at the *default*, Druid uses the bundled `bin/run-java` script if it is present in the working directory (it honors the `DRUID_JAVA_HOME`/`JAVA_HOME` environment variables), otherwise it falls back to `java` on the `PATH`. |java| |
There was a problem hiding this comment.
| |`druid.indexer.runner.javaCommand`| Command required to execute java. When left at the *default*, Druid uses the bundled `bin/run-java` script if it is present in the working directory (it honors the `DRUID_JAVA_HOME`/`JAVA_HOME` environment variables), otherwise it falls back to `java` on the `PATH`. |java| | |
| |`druid.indexer.runner.javaCommand`| Command required to execute java. If not specified, Druid uses the bundled `bin/run-java` script if it is present in the working directory (it honors the `DRUID_JAVA_HOME`/`JAVA_HOME` environment variables), otherwise it falls back to `java` on the `PATH`. |java| |
| */ | ||
| public static String getJavaCommand(String configuredJavaCommand, File workingDir) | ||
| { | ||
| if (ForkingTaskRunnerConfig.DEFAULT_JAVA_COMMAND.equals(configuredJavaCommand) |
There was a problem hiding this comment.
How can we distinguish between the case where the user has actually specified java as the command vs the default?
There was a problem hiding this comment.
One thing we could do is not assign any default in the config object, and do the following in this method:
- if user has specified a command (i.e. if config.getJavaCommand is not null), use that.
- if
bin/run-javaexists, use that - use
java
This way, the entire fallback logic exists in one place.
| { | ||
| final File workingDir = temporaryFolder.newFolder(); | ||
| final File binDir = new File(workingDir, "bin"); | ||
| Assert.assertTrue(binDir.mkdirs()); |
There was a problem hiding this comment.
to avoid use of forbidden API
| Assert.assertTrue(binDir.mkdirs()); | |
| Assert.assertTrue(FileUtils(binDir.mkdirp())); |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 4 of 4 changed files.
This is an automated review by Codex GPT-5.6-Sol
| public static String getJavaCommand(String configuredJavaCommand, File workingDir) | ||
| { | ||
| if (ForkingTaskRunnerConfig.DEFAULT_JAVA_COMMAND.equals(configuredJavaCommand) | ||
| && new File(workingDir, RUN_JAVA_COMMAND).exists()) { |
There was a problem hiding this comment.
[P2] Check launcher executability before selecting it
exists() also accepts non-executable files and directories. In installations where mode bits were not preserved, peon startup now selects bin/run-java and fails in ProcessBuilder instead of falling back to the working java command on PATH. Require a regular executable file before returning the bundled launcher, and cover the non-executable case.
|
Thanks for the review @kfaraz . Addressed all your comments. Please take a look since this should probably be going in druid 38 as we have bumped up the min JDK version to 21. |
3 participants
The druid documentation mentions setting JAVA_HOME should be sufficient for all druid processes to use the updated java versions.
The launched peons from the middle manager were not honoring that.
Added logic so that middle managers start honoring JAVA_HOME by using the
bin/javascript which should be present in the working directory.Update logic is:
The existence check is reliable because peons inherit the parent process's working directory. The default config value and docs remain java; the doc note describes the auto-upgrade behavior.