You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by as...@apache.org on 2018/11/22 02:25:44 UTC
[incubator-druid] branch master updated: Fix missing default config
in some calls to coordinator dynamic configs. (#6652)
This is an automated email from the ASF dual-hosted git repository.
asdf2014 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git
The following commit(s) were added to refs/heads/master by this push:
new 92cce04 Fix missing default config in some calls to coordinator dynamic configs. (#6652)
92cce04 is described below
commit 92cce041654f48daffc0deb76d68e6afae3de8ee
Author: Gian Merlino <gi...@gmail.com>
AuthorDate: Wed Nov 21 21:25:39 2018 -0500
Fix missing default config in some calls to coordinator dynamic configs. (#6652)
* Fix missing default config in some calls to coordinator dynamic configs.
The lack of a default config meant that if someone called an API
_without_ a default config before one _with_ a default config, then
the default value would get stuck at null instead of the intended
default value. I noticed this in a cluster where calling /druid/coordinator/v1/config
before a coordinator had fully started up would lead to NPEs during
DruidCoordinatorRuleRunner.
This patch makes the default configs consistent across all calls.
* Remove unnecessary null check.
---
.../coordinator/CoordinatorCompactionConfig.java | 19 +++++++
.../coordinator/CoordinatorDynamicConfig.java | 29 +++++++++-
.../druid/server/coordinator/DruidCoordinator.java | 12 +---
.../http/CoordinatorCompactionConfigsResource.java | 65 ++++++----------------
.../http/CoordinatorDynamicConfigsResource.java | 23 +++-----
5 files changed, 71 insertions(+), 77 deletions(-)
diff --git a/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorCompactionConfig.java b/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorCompactionConfig.java
index bb16e43..cc1fdf6 100644
--- a/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorCompactionConfig.java
+++ b/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorCompactionConfig.java
@@ -21,11 +21,15 @@ package org.apache.druid.server.coordinator;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
+import org.apache.druid.common.config.JacksonConfigManager;
+import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.util.List;
import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
public class CoordinatorCompactionConfig
{
@@ -73,6 +77,21 @@ public class CoordinatorCompactionConfig
return new CoordinatorCompactionConfig(ImmutableList.of(), null, null);
}
+ public static AtomicReference<CoordinatorCompactionConfig> watch(final JacksonConfigManager configManager)
+ {
+ return configManager.watch(
+ CoordinatorCompactionConfig.CONFIG_KEY,
+ CoordinatorCompactionConfig.class,
+ CoordinatorCompactionConfig.empty()
+ );
+ }
+
+ @Nonnull
+ public static CoordinatorCompactionConfig current(final JacksonConfigManager configManager)
+ {
+ return Preconditions.checkNotNull(watch(configManager).get(), "Got null config from watcher?!");
+ }
+
@JsonCreator
public CoordinatorCompactionConfig(
@JsonProperty("compactionConfigs") List<DataSourceCompactionConfig> compactionConfigs,
diff --git a/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java b/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java
index 726f2fa..c51a04a 100644
--- a/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java
+++ b/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java
@@ -21,21 +21,25 @@ package org.apache.druid.server.coordinator;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
+import org.apache.druid.common.config.JacksonConfigManager;
import org.apache.druid.java.util.common.IAE;
+import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.util.Collection;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
/**
* This class is for users to change their configurations while their Druid cluster is running.
* These configurations are designed to allow only simple values rather than complicated JSON objects.
*
- * @see org.apache.druid.common.config.JacksonConfigManager
+ * @see JacksonConfigManager
* @see org.apache.druid.common.config.ConfigManager
*/
public class CoordinatorDynamicConfig
@@ -121,6 +125,21 @@ public class CoordinatorDynamicConfig
}
}
+ public static AtomicReference<CoordinatorDynamicConfig> watch(final JacksonConfigManager configManager)
+ {
+ return configManager.watch(
+ CoordinatorDynamicConfig.CONFIG_KEY,
+ CoordinatorDynamicConfig.class,
+ CoordinatorDynamicConfig.builder().build()
+ );
+ }
+
+ @Nonnull
+ public static CoordinatorDynamicConfig current(final JacksonConfigManager configManager)
+ {
+ return Preconditions.checkNotNull(watch(configManager).get(), "Got null config from watcher?!");
+ }
+
@JsonProperty
public long getMillisToWaitBeforeDeleting()
{
@@ -424,7 +443,9 @@ public class CoordinatorDynamicConfig
killDataSourceWhitelist,
killAllDataSources == null ? DEFAULT_KILL_ALL_DATA_SOURCES : killAllDataSources,
killPendingSegmentsSkipList,
- maxSegmentsInNodeLoadingQueue == null ? DEFAULT_MAX_SEGMENTS_IN_NODE_LOADING_QUEUE : maxSegmentsInNodeLoadingQueue
+ maxSegmentsInNodeLoadingQueue == null
+ ? DEFAULT_MAX_SEGMENTS_IN_NODE_LOADING_QUEUE
+ : maxSegmentsInNodeLoadingQueue
);
}
@@ -442,7 +463,9 @@ public class CoordinatorDynamicConfig
killDataSourceWhitelist == null ? defaults.getKillDataSourceWhitelist() : killDataSourceWhitelist,
killAllDataSources == null ? defaults.isKillAllDataSources() : killAllDataSources,
killPendingSegmentsSkipList == null ? defaults.getKillPendingSegmentsSkipList() : killPendingSegmentsSkipList,
- maxSegmentsInNodeLoadingQueue == null ? defaults.getMaxSegmentsInNodeLoadingQueue() : maxSegmentsInNodeLoadingQueue
+ maxSegmentsInNodeLoadingQueue == null
+ ? defaults.getMaxSegmentsInNodeLoadingQueue()
+ : maxSegmentsInNodeLoadingQueue
);
}
}
diff --git a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
index 8bf9ed5..8520683 100644
--- a/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
+++ b/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
@@ -324,20 +324,12 @@ public class DruidCoordinator
public CoordinatorDynamicConfig getDynamicConfigs()
{
- return configManager.watch(
- CoordinatorDynamicConfig.CONFIG_KEY,
- CoordinatorDynamicConfig.class,
- CoordinatorDynamicConfig.builder().build()
- ).get();
+ return CoordinatorDynamicConfig.current(configManager);
}
public CoordinatorCompactionConfig getCompactionConfig()
{
- return configManager.watch(
- CoordinatorCompactionConfig.CONFIG_KEY,
- CoordinatorCompactionConfig.class,
- CoordinatorCompactionConfig.empty()
- ).get();
+ return CoordinatorCompactionConfig.current(configManager);
}
public void removeSegment(DataSegment segment)
diff --git a/server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java b/server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
index 4604403..8f7bf26 100644
--- a/server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
+++ b/server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
@@ -65,12 +65,7 @@ public class CoordinatorCompactionConfigsResource
@Produces(MediaType.APPLICATION_JSON)
public Response getCompactConfig()
{
- return Response.ok(
- manager.watch(
- CoordinatorCompactionConfig.CONFIG_KEY,
- CoordinatorCompactionConfig.class
- ).get()
- ).build();
+ return Response.ok(CoordinatorCompactionConfig.current(manager)).build();
}
@POST
@@ -84,17 +79,13 @@ public class CoordinatorCompactionConfigsResource
@Context HttpServletRequest req
)
{
- CoordinatorCompactionConfig current = manager.watch(
- CoordinatorCompactionConfig.CONFIG_KEY,
- CoordinatorCompactionConfig.class
- ).get();
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
- final CoordinatorCompactionConfig newCompactionConfig;
- if (current != null) {
- newCompactionConfig = CoordinatorCompactionConfig.from(current, compactionTaskSlotRatio, maxCompactionTaskSlots);
- } else {
- newCompactionConfig = new CoordinatorCompactionConfig(null, compactionTaskSlotRatio, maxCompactionTaskSlots);
- }
+ final CoordinatorCompactionConfig newCompactionConfig = CoordinatorCompactionConfig.from(
+ current,
+ compactionTaskSlotRatio,
+ maxCompactionTaskSlots
+ );
final SetResult setResult = manager.set(
CoordinatorCompactionConfig.CONFIG_KEY,
@@ -120,22 +111,14 @@ public class CoordinatorCompactionConfigsResource
@Context HttpServletRequest req
)
{
- CoordinatorCompactionConfig current = manager.watch(
- CoordinatorCompactionConfig.CONFIG_KEY,
- CoordinatorCompactionConfig.class
- ).get();
-
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
final CoordinatorCompactionConfig newCompactionConfig;
- if (current != null) {
- final Map<String, DataSourceCompactionConfig> newConfigs = current
- .getCompactionConfigs()
- .stream()
- .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
- newConfigs.put(newConfig.getDataSource(), newConfig);
- newCompactionConfig = CoordinatorCompactionConfig.from(current, ImmutableList.copyOf(newConfigs.values()));
- } else {
- newCompactionConfig = CoordinatorCompactionConfig.from(ImmutableList.of(newConfig));
- }
+ final Map<String, DataSourceCompactionConfig> newConfigs = current
+ .getCompactionConfigs()
+ .stream()
+ .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+ newConfigs.put(newConfig.getDataSource(), newConfig);
+ newCompactionConfig = CoordinatorCompactionConfig.from(current, ImmutableList.copyOf(newConfigs.values()));
final SetResult setResult = manager.set(
CoordinatorCompactionConfig.CONFIG_KEY,
@@ -155,15 +138,7 @@ public class CoordinatorCompactionConfigsResource
@Produces(MediaType.APPLICATION_JSON)
public Response getCompactionConfig(@PathParam("dataSource") String dataSource)
{
- CoordinatorCompactionConfig current = manager.watch(
- CoordinatorCompactionConfig.CONFIG_KEY,
- CoordinatorCompactionConfig.class
- ).get();
-
- if (current == null) {
- return Response.status(Response.Status.NOT_FOUND).build();
- }
-
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
final Map<String, DataSourceCompactionConfig> configs = current
.getCompactionConfigs()
.stream()
@@ -187,15 +162,7 @@ public class CoordinatorCompactionConfigsResource
@Context HttpServletRequest req
)
{
- CoordinatorCompactionConfig current = manager.watch(
- CoordinatorCompactionConfig.CONFIG_KEY,
- CoordinatorCompactionConfig.class
- ).get();
-
- if (current == null) {
- return Response.status(Response.Status.NOT_FOUND).build();
- }
-
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
final Map<String, DataSourceCompactionConfig> configs = current
.getCompactionConfigs()
.stream()
diff --git a/server/src/main/java/org/apache/druid/server/http/CoordinatorDynamicConfigsResource.java b/server/src/main/java/org/apache/druid/server/http/CoordinatorDynamicConfigsResource.java
index 1372d85..85bdd33 100644
--- a/server/src/main/java/org/apache/druid/server/http/CoordinatorDynamicConfigsResource.java
+++ b/server/src/main/java/org/apache/druid/server/http/CoordinatorDynamicConfigsResource.java
@@ -67,31 +67,24 @@ public class CoordinatorDynamicConfigsResource
@Produces(MediaType.APPLICATION_JSON)
public Response getDynamicConfigs()
{
- return Response.ok(
- manager.watch(
- CoordinatorDynamicConfig.CONFIG_KEY,
- CoordinatorDynamicConfig.class
- ).get()
- ).build();
+ return Response.ok(CoordinatorDynamicConfig.current(manager)).build();
}
// default value is used for backwards compatibility
@POST
@Consumes(MediaType.APPLICATION_JSON)
- public Response setDynamicConfigs(final CoordinatorDynamicConfig.Builder dynamicConfigBuilder,
- @HeaderParam(AuditManager.X_DRUID_AUTHOR) @DefaultValue("") final String author,
- @HeaderParam(AuditManager.X_DRUID_COMMENT) @DefaultValue("") final String comment,
- @Context HttpServletRequest req
+ public Response setDynamicConfigs(
+ final CoordinatorDynamicConfig.Builder dynamicConfigBuilder,
+ @HeaderParam(AuditManager.X_DRUID_AUTHOR) @DefaultValue("") final String author,
+ @HeaderParam(AuditManager.X_DRUID_COMMENT) @DefaultValue("") final String comment,
+ @Context HttpServletRequest req
)
{
- CoordinatorDynamicConfig current = manager.watch(
- CoordinatorDynamicConfig.CONFIG_KEY,
- CoordinatorDynamicConfig.class
- ).get();
+ CoordinatorDynamicConfig current = CoordinatorDynamicConfig.current(manager);
final SetResult setResult = manager.set(
CoordinatorDynamicConfig.CONFIG_KEY,
- current == null ? dynamicConfigBuilder.build() : dynamicConfigBuilder.build(current),
+ dynamicConfigBuilder.build(current),
new AuditInfo(author, comment, req.getRemoteAddr())
);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org