kfaraz commented on a change in pull request #12326:
URL: https://github.com/apache/druid/pull/12326#discussion_r834515642
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/task/NoopTask.java
##########
@@ -176,6 +178,14 @@ public static NoopTask withGroupId(String groupId)
return new NoopTask(null, groupId, null, 0, 0, null, null, null);
}
+ public NoopTask withJavaOptsContext(String javaOpts, List<String>
javaOptsArray)
Review comment:
This approach is not advisable.
This is not a common use case and is being used only by the test.
The `mapper.readValue()` approach suggested by @rohangarg would be better if
we need to do this.
##########
File path:
indexing-service/src/test/java/org/apache/druid/indexing/overlord/ForkingTaskRunnerTest.java
##########
@@ -353,4 +353,77 @@ int waitForTaskProcessToComplete(Task task, ProcessHolder
processHolder, File lo
Assert.assertEquals(TaskState.FAILED, status.getStatusCode());
Assert.assertEquals("task failure test", status.getErrorMsg());
}
+
+ @Test
+ public void testJavaOptsAndJavaOptsArrayOverride() throws
ExecutionException, InterruptedException
+ {
+ ObjectMapper mapper = new DefaultObjectMapper();
+ String javaOpts = "-Xmx1g -Xms1g";
+ List<String> javaOptsArray = ImmutableList.of("-Xmx10g", "-Xms10g");
+ Task task = NoopTask.create().withJavaOptsContext(javaOpts, javaOptsArray);
+ ForkingTaskRunner forkingTaskRunner = new ForkingTaskRunner(
+ new ForkingTaskRunnerConfig(),
+ new TaskConfig(
+ null,
+ null,
+ null,
+ null,
+ ImmutableList.of(),
+ false,
+ new Period("PT0S"),
+ new Period("PT10S"),
+ ImmutableList.of(),
+ false,
+ false,
+ TaskConfig.BATCH_PROCESSING_MODE_DEFAULT.name()
+ ),
+ new WorkerConfig(),
+ new Properties(),
+ new NoopTaskLogs(),
+ mapper,
+ new DruidNode("middleManager", "host", false, 8091, null, true, false),
+ new StartupLoggingConfig()
+ )
+ {
+ @Override
+ ProcessHolder runTaskProcess(List<String> command, File logFile,
TaskLocation taskLocation) throws IOException
+ {
+ ProcessHolder processHolder = Mockito.mock(ProcessHolder.class);
+
Mockito.doNothing().when(processHolder).registerWithCloser(ArgumentMatchers.any());
+ Mockito.doNothing().when(processHolder).shutdown();
+
+ int xmxJavaOptsIndex = 0;
+ int xmxJavaOptsArrayIndex = 0;
+ String statusPath = "status.json";
+ for (int i = 0; i < command.size(); i++) {
+ if (command.get(i).endsWith("status.json")) {
+ statusPath = command.get(i);
+ }
+
+ if ("-Xmx1g".equals(command.get(i))) {
+ xmxJavaOptsIndex = i;
+ }
+ if ("-Xmx10g".equals(command.get(i))) {
+ xmxJavaOptsArrayIndex = i;
+ }
+ }
+ if (0 < xmxJavaOptsIndex && xmxJavaOptsIndex < xmxJavaOptsArrayIndex) {
+ mapper.writeValue(new File(statusPath),
TaskStatus.success(task.getId()));
+ } else {
+ mapper.writeValue(new File(statusPath),
TaskStatus.failure(task.getId(), "javaOpts or javaOptsArray override failed"));
+ }
+
+ return processHolder;
+ }
+
+ @Override
+ int waitForTaskProcessToComplete(Task task, ProcessHolder processHolder,
File logFile, File reportsFile)
+ {
+ return 0;
Review comment:
Return a non-zero exit code here to immediately fail the task.
With a zero exit code, the runner tries to read the status file.
##########
File path:
indexing-service/src/test/java/org/apache/druid/indexing/overlord/ForkingTaskRunnerTest.java
##########
@@ -353,4 +353,77 @@ int waitForTaskProcessToComplete(Task task, ProcessHolder
processHolder, File lo
Assert.assertEquals(TaskState.FAILED, status.getStatusCode());
Assert.assertEquals("task failure test", status.getErrorMsg());
}
+
+ @Test
+ public void testJavaOptsAndJavaOptsArrayOverride() throws
ExecutionException, InterruptedException
+ {
+ ObjectMapper mapper = new DefaultObjectMapper();
+ String javaOpts = "-Xmx1g -Xms1g";
+ List<String> javaOptsArray = ImmutableList.of("-Xmx10g", "-Xms10g");
+ Task task = NoopTask.create().withJavaOptsContext(javaOpts, javaOptsArray);
+ ForkingTaskRunner forkingTaskRunner = new ForkingTaskRunner(
+ new ForkingTaskRunnerConfig(),
+ new TaskConfig(
+ null,
+ null,
+ null,
+ null,
+ ImmutableList.of(),
+ false,
+ new Period("PT0S"),
+ new Period("PT10S"),
+ ImmutableList.of(),
+ false,
+ false,
+ TaskConfig.BATCH_PROCESSING_MODE_DEFAULT.name()
+ ),
+ new WorkerConfig(),
+ new Properties(),
+ new NoopTaskLogs(),
+ mapper,
+ new DruidNode("middleManager", "host", false, 8091, null, true, false),
+ new StartupLoggingConfig()
+ )
+ {
+ @Override
+ ProcessHolder runTaskProcess(List<String> command, File logFile,
TaskLocation taskLocation) throws IOException
+ {
+ ProcessHolder processHolder = Mockito.mock(ProcessHolder.class);
+
Mockito.doNothing().when(processHolder).registerWithCloser(ArgumentMatchers.any());
+ Mockito.doNothing().when(processHolder).shutdown();
+
+ int xmxJavaOptsIndex = 0;
+ int xmxJavaOptsArrayIndex = 0;
+ String statusPath = "status.json";
+ for (int i = 0; i < command.size(); i++) {
+ if (command.get(i).endsWith("status.json")) {
+ statusPath = command.get(i);
+ }
+
+ if ("-Xmx1g".equals(command.get(i))) {
+ xmxJavaOptsIndex = i;
+ }
+ if ("-Xmx10g".equals(command.get(i))) {
+ xmxJavaOptsArrayIndex = i;
+ }
+ }
+ if (0 < xmxJavaOptsIndex && xmxJavaOptsIndex < xmxJavaOptsArrayIndex) {
Review comment:
This test can probably be much simpler.
The current implementation seems a little convoluted just to verify that a
argument A comes after argument B in the command list.
We need not write the status file at all. We just need to check that the
index of arg A > index of arg B.
Since the `runTaskProcess` might happen in a separate thread, use two
`AtomicInteger`s or a single `AtomicBoolean` to capture the relative position
of these two args and verify their values (instead of the task status).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
