samarthjain commented on a change in pull request #12097:
URL: https://github.com/apache/druid/pull/12097#discussion_r818152577
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java
##########
@@ -88,11 +89,19 @@
*/
void shutdown(String taskid, String reason);
- default void shutdown(String taskid, String reasonFormat, Object... args)
+ default void shutdown(String taskid, String reasonFormat, Object...
reasonArgs)
{
- shutdown(taskid, StringUtils.format(reasonFormat, args));
+ // only calculate the 'reason' string for debug level logging
+ // in large clusters the 'reasonArgs' may be very large / expensive if it
includes a list of tasks
+ String reason = (getLogger().isDebugEnabled()) ?
StringUtils.format(reasonFormat, reasonArgs) : "debug log disabled";
+ shutdown(taskid, reason);
}
+ /**
+ * Get the logger. Not expected to be called by consumers.
+ */
+ Logger getLogger();
Review comment:
I would probably change the default implementation to throw an
`UnsupportedOperationException`. This will also have the benefit of not having
to change classes implementing `TaskRunner` interface where they don't have a
logger available (like test classes).
##########
File path:
indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskRunnerTest.java
##########
@@ -0,0 +1,205 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.overlord;
+
+import com.google.common.base.Optional;
+import com.google.common.util.concurrent.ListenableFuture;
+import org.apache.druid.indexer.TaskStatus;
+import org.apache.druid.indexing.common.task.Task;
+import org.apache.druid.indexing.overlord.autoscaling.ScalingStats;
+import org.apache.druid.java.util.common.Pair;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.easymock.EasyMock;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+public class TaskRunnerTest
+{
+ @Test
+ public void testShutdownWithInfoOnConstructsString()
+ {
+ final ToStringMock expensiveObject = new ToStringMock();
+
+ Logger infoLogger = EasyMock.createMock(Logger.class);
+ EasyMock.expect(infoLogger.isDebugEnabled()).andReturn(true).atLeastOnce();
+ EasyMock.replay(infoLogger);
+
+ MockTaskRunner taskRunner = new MockTaskRunner(infoLogger);
+ taskRunner.shutdown("taskid", "reason %s", new Object[]{expensiveObject});
+
+ Assert.assertTrue(expensiveObject.getWasCalled());
+ }
+
+ @Test
+ public void testShutdownWithInfoOffDoesNotCallToString()
+ {
+ final ToStringMock expensiveObject = new ToStringMock();
+
+ Logger infoLogger = EasyMock.createMock(Logger.class);
+
EasyMock.expect(infoLogger.isDebugEnabled()).andReturn(false).atLeastOnce();
+ EasyMock.replay(infoLogger);
+
+ MockTaskRunner taskRunner = new MockTaskRunner(infoLogger);
+ taskRunner.shutdown("taskid", "reason %s", new Object[]{expensiveObject});
+
+ Assert.assertFalse(expensiveObject.getWasCalled());
+ }
+
+ /**
+ * EasyMock does not support mocking of toString, so this provides a custom
+ * object implementation to track whether toString was called.
+ */
+ public static class ToStringMock
+ {
+ AtomicBoolean wasCalled = new AtomicBoolean(false);
+
+ public boolean getWasCalled()
Review comment:
nit: maybe a better method name would be `wasCalled()` ?
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java
##########
@@ -88,11 +89,19 @@
*/
void shutdown(String taskid, String reason);
- default void shutdown(String taskid, String reasonFormat, Object... args)
+ default void shutdown(String taskid, String reasonFormat, Object...
reasonArgs)
{
- shutdown(taskid, StringUtils.format(reasonFormat, args));
+ // only calculate the 'reason' string for debug level logging
+ // in large clusters the 'reasonArgs' may be very large / expensive if it
includes a list of tasks
+ String reason = (getLogger().isDebugEnabled()) ?
StringUtils.format(reasonFormat, reasonArgs) : "debug log disabled";
Review comment:
Similarly, this -
https://github.com/apache/druid/blob/master/indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java#L299
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java
##########
@@ -88,11 +89,19 @@
*/
void shutdown(String taskid, String reason);
- default void shutdown(String taskid, String reasonFormat, Object... args)
+ default void shutdown(String taskid, String reasonFormat, Object...
reasonArgs)
{
- shutdown(taskid, StringUtils.format(reasonFormat, args));
+ // only calculate the 'reason' string for debug level logging
+ // in large clusters the 'reasonArgs' may be very large / expensive if it
includes a list of tasks
+ String reason = (getLogger().isDebugEnabled()) ?
StringUtils.format(reasonFormat, reasonArgs) : "debug log disabled";
Review comment:
Looking at the callers who pass in taskIds, I see only
TaskQueue#shutdown() that is passing in the task ids. So, I propose we modify
code in TaskQueue#shutdown something like this:
```
// Kill tasks that shouldn't be running
final Set<String> knownTaskIds = tasks
.stream()
.map(Task::getId)
.collect(Collectors.toSet());
final Set<String> tasksToKill =
Sets.difference(runnerTaskFutures.keySet(), knownTaskIds);
if (!tasksToKill.isEmpty()) {
log.info("Asking taskRunner to clean up %,d tasks.",
tasksToKill.size());
// On large installations running several thousands of tasks,
// concatenating the list of known task ids can be compupationally
expensive.
boolean logKnownTaskIds = log.isDebugEnabled();
String reason = logKnownTaskIds
? String.format("Task is not in knownTaskIds[%s]",
knownTaskIds)
: "Task is not in knownTaskIds";
for (final String taskId : tasksToKill) {
try {
taskRunner.shutdown(
taskId, reason
);
}
catch (Exception e) {
log.warn(e, "TaskRunner failed to clean up task: %s", taskId);
}
}
}
```
If we do this, then you won't have to add a new `getLogger()` method in
`TaskRunner` interface.
##########
File path:
indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskRunnerTest.java
##########
@@ -0,0 +1,205 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.overlord;
+
+import com.google.common.base.Optional;
+import com.google.common.util.concurrent.ListenableFuture;
+import org.apache.druid.indexer.TaskStatus;
+import org.apache.druid.indexing.common.task.Task;
+import org.apache.druid.indexing.overlord.autoscaling.ScalingStats;
+import org.apache.druid.java.util.common.Pair;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.easymock.EasyMock;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+public class TaskRunnerTest
+{
+ @Test
+ public void testShutdownWithInfoOnConstructsString()
+ {
+ final ToStringMock expensiveObject = new ToStringMock();
+
+ Logger infoLogger = EasyMock.createMock(Logger.class);
+ EasyMock.expect(infoLogger.isDebugEnabled()).andReturn(true).atLeastOnce();
+ EasyMock.replay(infoLogger);
+
+ MockTaskRunner taskRunner = new MockTaskRunner(infoLogger);
+ taskRunner.shutdown("taskid", "reason %s", new Object[]{expensiveObject});
+
+ Assert.assertTrue(expensiveObject.getWasCalled());
+ }
+
+ @Test
+ public void testShutdownWithInfoOffDoesNotCallToString()
+ {
+ final ToStringMock expensiveObject = new ToStringMock();
+
+ Logger infoLogger = EasyMock.createMock(Logger.class);
+
EasyMock.expect(infoLogger.isDebugEnabled()).andReturn(false).atLeastOnce();
+ EasyMock.replay(infoLogger);
+
+ MockTaskRunner taskRunner = new MockTaskRunner(infoLogger);
+ taskRunner.shutdown("taskid", "reason %s", new Object[]{expensiveObject});
+
+ Assert.assertFalse(expensiveObject.getWasCalled());
+ }
+
+ /**
+ * EasyMock does not support mocking of toString, so this provides a custom
+ * object implementation to track whether toString was called.
+ */
+ public static class ToStringMock
Review comment:
nit: We should probably have these inner classes `ToStringMock` and
`MockTaskRunner` as private static.
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskRunner.java
##########
@@ -88,11 +89,19 @@
*/
void shutdown(String taskid, String reason);
- default void shutdown(String taskid, String reasonFormat, Object... args)
+ default void shutdown(String taskid, String reasonFormat, Object...
reasonArgs)
{
- shutdown(taskid, StringUtils.format(reasonFormat, args));
+ // only calculate the 'reason' string for debug level logging
+ // in large clusters the 'reasonArgs' may be very large / expensive if it
includes a list of tasks
+ String reason = (getLogger().isDebugEnabled()) ?
StringUtils.format(reasonFormat, reasonArgs) : "debug log disabled";
Review comment:
If debug logging is not enabled, then the following log line won't be
reported.
https://github.com/apache/druid/blob/master/indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java#L582
--
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]
