You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org> on 2023/05/10 07:16:17 UTC
[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #14239: Be able to load segments on Peons
github-code-scanning[bot] commented on code in PR #14239:
URL: https://github.com/apache/druid/pull/14239#discussion_r1189456536
##########
server/src/test/java/org/apache/druid/server/log/LoggingRequestLoggerProviderTest.java:
##########
@@ -77,7 +77,7 @@
final Properties properties = new Properties();
properties.put(propertyPrefix + ".type", "noop");
provider.inject(properties, injector.getInstance(JsonConfigurator.class));
- Assert.assertThat(provider.get().get().get(), Matchers.instanceOf(NoopRequestLogger.class));
+ Assert.assertThat(provider.get().get(), Matchers.instanceOf(NoopRequestLogger.class));
Review Comment:
## Deprecated method or constructor invocation
Invoking [Assert.assertThat](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/4934)
##########
server/src/test/java/org/apache/druid/client/cache/CacheConfigTest.java:
##########
@@ -126,7 +126,7 @@
{
properties.put(PROPERTY_PREFIX + ".numBackgroundThreads", "BABBA YAGA");
configProvider.inject(properties, configurator);
- CacheConfig config = configProvider.get().get();
+ CacheConfig config = configProvider.get();
Review Comment:
## Unread local variable
Variable 'CacheConfig config' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/4935)
##########
server/src/test/java/org/apache/druid/client/cache/CacheConfigTest.java:
##########
@@ -154,7 +154,7 @@
{
properties.put(PROPERTY_PREFIX + ".populateCache", "FaLse");
configProvider.inject(properties, configurator);
- CacheConfig config = configProvider.get().get();
+ CacheConfig config = configProvider.get();
Review Comment:
## Unread local variable
Variable 'CacheConfig config' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/4938)
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ThreadingTaskRunner.java:
##########
@@ -167,7 +167,7 @@
);
}
- final File taskDir = new File(baseDirForTask, task.getId());
+ final File taskDir = new File(storageSlot.getDirectory(), task.getId());
Review Comment:
## Uncontrolled data used in path expression
This path depends on a [user-provided value](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/4940)
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java:
##########
@@ -154,20 +153,19 @@
@Override
public TaskStatus call()
{
-
- final File baseDirForTask;
+ final TaskStorageDirTracker.StorageSlot storageSlot;
try {
- baseDirForTask = getTracker().pickBaseDir(task.getId());
+ storageSlot = getTracker().pickStorageSlot(task.getId());
}
catch (RuntimeException e) {
- LOG.error(e, "Failed to get directory for task [%s], cannot schedule.", task.getId());
+ LOG.warn(e, "Failed to get storage slot for task [%s], cannot schedule.", task.getId());
return TaskStatus.failure(
task.getId(),
- StringUtils.format("Could not schedule due to error [%s]", e.getMessage())
+ StringUtils.format("Failed to get storage slot due to error [%s]", e.getMessage())
);
}
- final File taskDir = new File(baseDirForTask, task.getId());
+ final File taskDir = new File(storageSlot.getDirectory(), task.getId());
Review Comment:
## Uncontrolled data used in path expression
This path depends on a [user-provided value](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/4939)
##########
server/src/test/java/org/apache/druid/client/cache/CacheConfigTest.java:
##########
@@ -144,7 +144,7 @@
{
properties.put(PROPERTY_PREFIX + ".populateCache", "FALSE");
configProvider.inject(properties, configurator);
- CacheConfig config = configProvider.get().get();
+ CacheConfig config = configProvider.get();
Review Comment:
## Unread local variable
Variable 'CacheConfig config' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/4937)
##########
server/src/test/java/org/apache/druid/client/cache/CacheConfigTest.java:
##########
@@ -135,7 +135,7 @@
{
properties.put(PROPERTY_PREFIX + ".populateCache", "TRUE");
configProvider.inject(properties, configurator);
- CacheConfig config = configProvider.get().get();
+ CacheConfig config = configProvider.get();
Review Comment:
## Unread local variable
Variable 'CacheConfig config' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/4936)
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskStorageDirTracker.java:
##########
@@ -24,91 +24,202 @@
import org.apache.druid.indexing.worker.config.WorkerConfig;
import org.apache.druid.java.util.common.FileUtils;
import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.java.util.common.lifecycle.LifecycleStart;
import javax.annotation.Nullable;
import java.io.File;
import java.io.IOException;
+import java.util.Arrays;
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;
+/**
+ * Used to pick storage slots for tasks when run from the middle manager.
+ */
public class TaskStorageDirTracker
{
public static TaskStorageDirTracker fromConfigs(WorkerConfig workerConfig, TaskConfig taskConfig)
{
+ final List<File> baseTaskDirs;
if (workerConfig == null) {
- return new TaskStorageDirTracker(ImmutableList.of(taskConfig.getBaseTaskDir()));
+ baseTaskDirs = ImmutableList.of(taskConfig.getBaseTaskDir());
} else {
final List<String> basePaths = workerConfig.getBaseTaskDirs();
if (basePaths == null) {
- return new TaskStorageDirTracker(ImmutableList.of(taskConfig.getBaseTaskDir()));
+ baseTaskDirs = ImmutableList.of(taskConfig.getBaseTaskDir());
+ } else {
+ baseTaskDirs = basePaths.stream().map(File::new).collect(Collectors.toList());
}
- return new TaskStorageDirTracker(
- basePaths.stream().map(File::new).collect(Collectors.toList())
- );
}
+
+ return fromBaseDirs(baseTaskDirs, workerConfig.getCapacity(), workerConfig.getBaseTaskDirSize());
Review Comment:
## Dereferenced variable may be null
Variable [workerConfig](1) may be null at this access as suggested by [this](2) null guard.
[Show more details](https://github.com/apache/druid/security/code-scanning/4941)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org