From cf8bfb91178c21e55e1465020410c2487c55c819 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Tue, 4 Mar 2025 21:37:38 +0530 Subject: [PATCH 1/5] Remove usages of skife config --- docs/configuration/logging.md | 3 - examples/conf/druid/auto/_common/log4j2.xml | 3 - .../conf/druid/cluster/_common/log4j2.xml | 3 - .../single-server/large/_common/log4j2.xml | 3 - .../single-server/medium/_common/log4j2.xml | 3 - .../micro-quickstart/_common/log4j2.xml | 3 - .../nano-quickstart/_common/log4j2.xml | 3 - .../single-server/small/_common/log4j2.xml | 3 - .../single-server/xlarge/_common/log4j2.xml | 3 - .../druid/testsEx/config/Initializer.java | 5 +- licenses.yaml | 10 --- pom.xml | 11 --- processing/pom.xml | 4 - .../org/apache/druid/guice/ConfigModule.java | 10 --- .../apache/druid/guice/ConfigProvider.java | 87 ------------------- .../druid/guice/DruidSecondaryModule.java | 5 -- .../druid/java/util/common/config/Config.java | 36 -------- .../util/common/config/DurationCoercible.java | 47 ---------- server/pom.xml | 4 - ...LegacyBrokerParallelMergeConfigModule.java | 42 --------- .../query/BrokerParallelMergeConfig.java | 54 ++++++------ .../LegacyBrokerParallelMergeConfig.java | 74 ---------------- .../query/LegacyBrokerPoolMergeConfig.java | 67 ++++++++++++++ .../query/LegacyBrokerTaskMergeConfig.java | 64 ++++++++++++++ .../guice/BrokerProcessingModuleTest.java | 81 +++++++++++++---- .../druid/guice/QueryableModuleTest.java | 1 - .../java/org/apache/druid/cli/CliBroker.java | 2 - 27 files changed, 226 insertions(+), 405 deletions(-) delete mode 100644 processing/src/main/java/org/apache/druid/guice/ConfigProvider.java delete mode 100644 processing/src/main/java/org/apache/druid/java/util/common/config/Config.java delete mode 100644 processing/src/main/java/org/apache/druid/java/util/common/config/DurationCoercible.java delete mode 100644 server/src/main/java/org/apache/druid/guice/LegacyBrokerParallelMergeConfigModule.java delete mode 100644 server/src/main/java/org/apache/druid/query/LegacyBrokerParallelMergeConfig.java create mode 100644 server/src/main/java/org/apache/druid/query/LegacyBrokerPoolMergeConfig.java create mode 100644 server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java diff --git a/docs/configuration/logging.md b/docs/configuration/logging.md index 0764ffcf862e..36b3492bb9e0 100644 --- a/docs/configuration/logging.md +++ b/docs/configuration/logging.md @@ -95,9 +95,6 @@ The following example log4j2.xml is based upon the micro quickstart: - - - diff --git a/examples/conf/druid/auto/_common/log4j2.xml b/examples/conf/druid/auto/_common/log4j2.xml index eb24a2a89406..b9f456c43eef 100644 --- a/examples/conf/druid/auto/_common/log4j2.xml +++ b/examples/conf/druid/auto/_common/log4j2.xml @@ -76,9 +76,6 @@ - - - diff --git a/examples/conf/druid/cluster/_common/log4j2.xml b/examples/conf/druid/cluster/_common/log4j2.xml index eb24a2a89406..b9f456c43eef 100644 --- a/examples/conf/druid/cluster/_common/log4j2.xml +++ b/examples/conf/druid/cluster/_common/log4j2.xml @@ -76,9 +76,6 @@ - - - diff --git a/examples/conf/druid/single-server/large/_common/log4j2.xml b/examples/conf/druid/single-server/large/_common/log4j2.xml index eb24a2a89406..b9f456c43eef 100644 --- a/examples/conf/druid/single-server/large/_common/log4j2.xml +++ b/examples/conf/druid/single-server/large/_common/log4j2.xml @@ -76,9 +76,6 @@ - - - diff --git a/examples/conf/druid/single-server/medium/_common/log4j2.xml b/examples/conf/druid/single-server/medium/_common/log4j2.xml index eb24a2a89406..b9f456c43eef 100644 --- a/examples/conf/druid/single-server/medium/_common/log4j2.xml +++ b/examples/conf/druid/single-server/medium/_common/log4j2.xml @@ -76,9 +76,6 @@ - - - diff --git a/examples/conf/druid/single-server/micro-quickstart/_common/log4j2.xml b/examples/conf/druid/single-server/micro-quickstart/_common/log4j2.xml index eb24a2a89406..b9f456c43eef 100644 --- a/examples/conf/druid/single-server/micro-quickstart/_common/log4j2.xml +++ b/examples/conf/druid/single-server/micro-quickstart/_common/log4j2.xml @@ -76,9 +76,6 @@ - - - diff --git a/examples/conf/druid/single-server/nano-quickstart/_common/log4j2.xml b/examples/conf/druid/single-server/nano-quickstart/_common/log4j2.xml index eb24a2a89406..b9f456c43eef 100644 --- a/examples/conf/druid/single-server/nano-quickstart/_common/log4j2.xml +++ b/examples/conf/druid/single-server/nano-quickstart/_common/log4j2.xml @@ -76,9 +76,6 @@ - - - diff --git a/examples/conf/druid/single-server/small/_common/log4j2.xml b/examples/conf/druid/single-server/small/_common/log4j2.xml index eb24a2a89406..b9f456c43eef 100644 --- a/examples/conf/druid/single-server/small/_common/log4j2.xml +++ b/examples/conf/druid/single-server/small/_common/log4j2.xml @@ -76,9 +76,6 @@ - - - diff --git a/examples/conf/druid/single-server/xlarge/_common/log4j2.xml b/examples/conf/druid/single-server/xlarge/_common/log4j2.xml index eb24a2a89406..b9f456c43eef 100644 --- a/examples/conf/druid/single-server/xlarge/_common/log4j2.xml +++ b/examples/conf/druid/single-server/xlarge/_common/log4j2.xml @@ -76,9 +76,6 @@ - - - diff --git a/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/config/Initializer.java b/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/config/Initializer.java index d420b65dd8df..200cfad17c70 100644 --- a/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/config/Initializer.java +++ b/integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/config/Initializer.java @@ -35,9 +35,9 @@ import org.apache.druid.discovery.DruidNodeDiscoveryProvider; import org.apache.druid.discovery.NodeRole; import org.apache.druid.guice.AnnouncerModule; +import org.apache.druid.guice.BrokerProcessingModule; import org.apache.druid.guice.JsonConfigProvider; import org.apache.druid.guice.LazySingleton; -import org.apache.druid.guice.LegacyBrokerParallelMergeConfigModule; import org.apache.druid.guice.ManageLifecycle; import org.apache.druid.guice.PolyBind; import org.apache.druid.guice.SQLMetadataStorageDruidModule; @@ -508,8 +508,7 @@ private static Injector makeInjector( new AnnouncerModule(), new DiscoveryModule(), // Dependencies from other modules - new LegacyBrokerParallelMergeConfigModule(), - // Dependencies from other modules + new BrokerProcessingModule(), new StorageNodeModule(), new MSQExternalDataSourceModule(), diff --git a/licenses.yaml b/licenses.yaml index b72b819d79e4..8f46132a18fe 100644 --- a/licenses.yaml +++ b/licenses.yaml @@ -2493,16 +2493,6 @@ libraries: --- -name: Config Magic -license_category: binary -module: java-core -license_name: Apache License version 2.0 -version: 0.9 -libraries: - - org.skife.config: config-magic - ---- - name: Apache Hadoop license_category: binary module: hadoop-client diff --git a/pom.xml b/pom.xml index fc7d78ccecb0..00d7492ceecf 100644 --- a/pom.xml +++ b/pom.xml @@ -388,17 +388,6 @@ airline 2.8.4 - - org.skife.config - config-magic - 0.9 - - - org.slf4j - slf4j-api - - - net.minidev json-smart diff --git a/processing/pom.xml b/processing/pom.xml index a320ad89c12a..2c985df48c49 100644 --- a/processing/pom.xml +++ b/processing/pom.xml @@ -65,10 +65,6 @@ com.fasterxml.jackson.dataformat jackson-dataformat-smile - - org.skife.config - config-magic - commons-io commons-io diff --git a/processing/src/main/java/org/apache/druid/guice/ConfigModule.java b/processing/src/main/java/org/apache/druid/guice/ConfigModule.java index 522ee63337e4..835999e634b2 100644 --- a/processing/src/main/java/org/apache/druid/guice/ConfigModule.java +++ b/processing/src/main/java/org/apache/druid/guice/ConfigModule.java @@ -21,13 +21,9 @@ import com.google.inject.Binder; import com.google.inject.Module; -import com.google.inject.Provides; -import org.apache.druid.java.util.common.config.Config; -import org.skife.config.ConfigurationObjectFactory; import javax.validation.Validation; import javax.validation.Validator; -import java.util.Properties; /** */ @@ -39,10 +35,4 @@ public void configure(Binder binder) binder.bind(Validator.class).toInstance(Validation.buildDefaultValidatorFactory().getValidator()); binder.bind(JsonConfigurator.class).in(LazySingleton.class); } - - @Provides @LazySingleton - public ConfigurationObjectFactory makeFactory(Properties props) - { - return Config.createFactory(props); - } } diff --git a/processing/src/main/java/org/apache/druid/guice/ConfigProvider.java b/processing/src/main/java/org/apache/druid/guice/ConfigProvider.java deleted file mode 100644 index 682dce0f7cb4..000000000000 --- a/processing/src/main/java/org/apache/druid/guice/ConfigProvider.java +++ /dev/null @@ -1,87 +0,0 @@ -/* - * 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.guice; - -import com.google.common.base.Preconditions; -import com.google.inject.Binder; -import com.google.inject.Inject; -import com.google.inject.Provider; -import org.apache.druid.java.util.common.logger.Logger; -import org.skife.config.ConfigurationObjectFactory; - -import java.util.Map; - -/** - * - */ -@Deprecated -public class ConfigProvider implements Provider -{ - private static final Logger log = new Logger(ConfigProvider.class); - - public static void bind(Binder binder, Class clazz) - { - binder.bind(clazz).toProvider(of(clazz)).in(LazySingleton.class); - } - - public static Provider of(Class clazz) - { - return of(clazz, null); - } - - public static Provider of(Class clazz, Map replacements) - { - return new ConfigProvider<>(clazz, replacements); - } - - private final Class clazz; - private final Map replacements; - - private ConfigurationObjectFactory factory = null; - - public ConfigProvider( - Class clazz, - Map replacements - ) - { - this.clazz = clazz; - this.replacements = replacements; - } - - @Inject - public void inject(ConfigurationObjectFactory factory) - { - this.factory = factory; - } - - @Override - public T get() - { - try { - // ConfigMagic handles a null replacements - Preconditions.checkNotNull(factory, "Code misconfigured, inject() didn't get called."); - return factory.buildWithReplacements(clazz, replacements); - } - catch (IllegalArgumentException e) { - log.info("Unable to build instance of class[%s]", clazz); - throw e; - } - } -} diff --git a/processing/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java b/processing/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java index a22ef051457b..be0ea786f58e 100644 --- a/processing/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java +++ b/processing/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java @@ -31,7 +31,6 @@ import org.apache.druid.guice.annotations.Json; import org.apache.druid.guice.annotations.JsonNonNull; import org.apache.druid.guice.annotations.Smile; -import org.skife.config.ConfigurationObjectFactory; import javax.validation.Validator; import java.util.Properties; @@ -40,7 +39,6 @@ public class DruidSecondaryModule implements Module { private final Properties properties; - private final ConfigurationObjectFactory factory; private final ObjectMapper jsonMapper; private final ObjectMapper jsonMapperOnlyNonNullValueSerialization; private final ObjectMapper smileMapper; @@ -49,7 +47,6 @@ public class DruidSecondaryModule implements Module @Inject public DruidSecondaryModule( Properties properties, - ConfigurationObjectFactory factory, @Json ObjectMapper jsonMapper, @JsonNonNull ObjectMapper jsonMapperOnlyNonNullValueSerialization, @Smile ObjectMapper smileMapper, @@ -57,7 +54,6 @@ public DruidSecondaryModule( ) { this.properties = properties; - this.factory = factory; this.jsonMapper = jsonMapper; this.jsonMapperOnlyNonNullValueSerialization = jsonMapperOnlyNonNullValueSerialization; this.smileMapper = smileMapper; @@ -69,7 +65,6 @@ public void configure(Binder binder) { binder.install(new DruidGuiceExtensions()); binder.bind(Properties.class).toInstance(properties); - binder.bind(ConfigurationObjectFactory.class).toInstance(factory); binder.bind(ObjectMapper.class).to(Key.get(ObjectMapper.class, Json.class)); binder.bind(Validator.class).toInstance(validator); binder.bind(JsonConfigurator.class); diff --git a/processing/src/main/java/org/apache/druid/java/util/common/config/Config.java b/processing/src/main/java/org/apache/druid/java/util/common/config/Config.java deleted file mode 100644 index e5a7c96275f0..000000000000 --- a/processing/src/main/java/org/apache/druid/java/util/common/config/Config.java +++ /dev/null @@ -1,36 +0,0 @@ -/* - * 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.java.util.common.config; - -import org.skife.config.ConfigurationObjectFactory; - -import java.util.Properties; - -/** -*/ -public class Config -{ - public static ConfigurationObjectFactory createFactory(Properties props) - { - ConfigurationObjectFactory configFactory = new ConfigurationObjectFactory(props); - configFactory.addCoercible(new DurationCoercible()); - return configFactory; - } -} diff --git a/processing/src/main/java/org/apache/druid/java/util/common/config/DurationCoercible.java b/processing/src/main/java/org/apache/druid/java/util/common/config/DurationCoercible.java deleted file mode 100644 index 5ba543bbfc08..000000000000 --- a/processing/src/main/java/org/apache/druid/java/util/common/config/DurationCoercible.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * 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.java.util.common.config; - -import org.joda.time.Duration; -import org.joda.time.Period; -import org.skife.config.Coercer; -import org.skife.config.Coercible; - -/** -*/ -public class DurationCoercible implements Coercible -{ - @Override - public Coercer accept(Class clazz) - { - if (Duration.class != clazz) { - return null; - } - - return new Coercer<>() - { - @Override - public Duration coerce(String value) - { - return new Period(value).toStandardDuration(); - } - }; - } -} diff --git a/server/pom.xml b/server/pom.xml index eb664b51a9fc..1dd27993e313 100644 --- a/server/pom.xml +++ b/server/pom.xml @@ -254,10 +254,6 @@ com.google.guava guava - - org.skife.config - config-magic - org.apache.curator curator-recipes diff --git a/server/src/main/java/org/apache/druid/guice/LegacyBrokerParallelMergeConfigModule.java b/server/src/main/java/org/apache/druid/guice/LegacyBrokerParallelMergeConfigModule.java deleted file mode 100644 index 3d44cf23f72b..000000000000 --- a/server/src/main/java/org/apache/druid/guice/LegacyBrokerParallelMergeConfigModule.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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.guice; - -import com.google.inject.Binder; -import com.google.inject.Module; -import org.apache.druid.query.LegacyBrokerParallelMergeConfig; - -/** - * Backwards compatibility for runtime.properties for Druid 27 and older to make deprecated config paths of - * {@link LegacyBrokerParallelMergeConfig} still work for Druid 28. - * {@link org.apache.druid.query.BrokerParallelMergeConfig} has replaced these configs, and will warn when these - * deprecated paths are configured. This module should be removed in Druid 29, along with - * {@link LegacyBrokerParallelMergeConfig} as well as the config-magic library that makes it work. - */ -@Deprecated -public class LegacyBrokerParallelMergeConfigModule implements Module -{ - - @Override - public void configure(Binder binder) - { - ConfigProvider.bind(binder, LegacyBrokerParallelMergeConfig.class); - } -} diff --git a/server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java b/server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java index 1f8d25a10c2d..a41fd1310989 100644 --- a/server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java +++ b/server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java @@ -19,10 +19,11 @@ package org.apache.druid.query; -import com.fasterxml.jackson.annotation.JacksonInject; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.annotations.VisibleForTesting; +import org.apache.druid.error.DruidException; +import org.apache.druid.error.InvalidInput; import org.apache.druid.java.util.common.guava.ParallelMergeCombiningSequence; import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.utils.JvmUtils; @@ -49,6 +50,12 @@ public class BrokerParallelMergeConfig @JsonProperty private final int smallBatchNumRows; + // These are needed to make JsonConfigurator happy + @JsonProperty + private final LegacyBrokerTaskMergeConfig task; + @JsonProperty + private final LegacyBrokerPoolMergeConfig pool; + @JsonCreator public BrokerParallelMergeConfig( @JsonProperty("useParallelMergePool") @Nullable Boolean useParallelMergePool, @@ -58,20 +65,22 @@ public BrokerParallelMergeConfig( @JsonProperty("targetRunTimeMillis") @Nullable Integer targetRunTimeMillis, @JsonProperty("initialYieldNumRows") @Nullable Integer initialYieldNumRows, @JsonProperty("smallBatchNumRows") @Nullable Integer smallBatchNumRows, - @JacksonInject LegacyBrokerParallelMergeConfig oldConfig + @JsonProperty("task") @Nullable LegacyBrokerTaskMergeConfig oldTaskConfig, + @JsonProperty("pool") @Nullable LegacyBrokerPoolMergeConfig oldPoolConfig ) { + this.task = oldTaskConfig; + this.pool = oldPoolConfig; if (parallelism == null) { - if (oldConfig == null || oldConfig.getMergePoolParallelism() == null) { + if (oldPoolConfig == null || oldPoolConfig.getMergePoolParallelism() == null) { // assume 2 hyper-threads per core, so that this value is probably by default the number // of physical cores * 1.5 this.parallelism = (int) Math.ceil(JvmUtils.getRuntimeInfo().getAvailableProcessors() * 0.75); } else { - warnDeprecated( + throw invalidConfigException( "druid.processing.merge.pool.parallelism", "druid.processing.merge.parallelism" ); - this.parallelism = oldConfig.getMergePoolParallelism(); } } else { this.parallelism = parallelism; @@ -91,70 +100,65 @@ public BrokerParallelMergeConfig( } if (awaitShutdownMillis == null) { - if (oldConfig == null || oldConfig.getMergePoolAwaitShutdownMillis() == null) { + if (oldPoolConfig == null || oldPoolConfig.getMergePoolAwaitShutdownMillis() == null) { this.awaitShutdownMillis = DEFAULT_MERGE_POOL_AWAIT_SHUTDOWN_MILLIS; } else { - warnDeprecated( + throw invalidConfigException( "druid.processing.merge.pool.awaitShutdownMillis", "druid.processing.merge.awaitShutdownMillis" ); - this.awaitShutdownMillis = oldConfig.getMergePoolAwaitShutdownMillis(); } } else { this.awaitShutdownMillis = awaitShutdownMillis; } if (defaultMaxQueryParallelism == null) { - if (oldConfig == null || oldConfig.getMergePoolDefaultMaxQueryParallelism() == null) { + if (oldPoolConfig == null || oldPoolConfig.getMergePoolDefaultMaxQueryParallelism() == null) { this.defaultMaxQueryParallelism = (int) Math.max(JvmUtils.getRuntimeInfo().getAvailableProcessors() * 0.5, 1); } else { - warnDeprecated( + throw invalidConfigException( "druid.processing.merge.pool.defaultMaxQueryParallelism", "druid.processing.merge.defaultMaxQueryParallelism" ); - this.defaultMaxQueryParallelism = oldConfig.getMergePoolDefaultMaxQueryParallelism(); } } else { this.defaultMaxQueryParallelism = defaultMaxQueryParallelism; } if (targetRunTimeMillis == null) { - if (oldConfig == null || oldConfig.getMergePoolTargetTaskRunTimeMillis() == null) { + if (oldTaskConfig == null || oldTaskConfig.getMergePoolTargetTaskRunTimeMillis() == null) { this.targetRunTimeMillis = ParallelMergeCombiningSequence.DEFAULT_TASK_TARGET_RUN_TIME_MILLIS; } else { - warnDeprecated( + throw invalidConfigException( "druid.processing.merge.task.targetRunTimeMillis", "druid.processing.merge.targetRunTimeMillis" ); - this.targetRunTimeMillis = oldConfig.getMergePoolTargetTaskRunTimeMillis(); } } else { this.targetRunTimeMillis = targetRunTimeMillis; } if (initialYieldNumRows == null) { - if (oldConfig == null || oldConfig.getMergePoolTaskInitialYieldRows() == null) { + if (oldTaskConfig == null || oldTaskConfig.getMergePoolTaskInitialYieldRows() == null) { this.initialYieldNumRows = ParallelMergeCombiningSequence.DEFAULT_TASK_INITIAL_YIELD_NUM_ROWS; } else { - warnDeprecated( + throw invalidConfigException( "druid.processing.merge.task.initialYieldNumRows", "druid.processing.merge.initialYieldNumRows" ); - this.initialYieldNumRows = oldConfig.getMergePoolTaskInitialYieldRows(); } } else { this.initialYieldNumRows = initialYieldNumRows; } if (smallBatchNumRows == null) { - if (oldConfig == null || oldConfig.getMergePoolSmallBatchRows() == null) { + if (oldTaskConfig == null || oldTaskConfig.getMergePoolSmallBatchRows() == null) { this.smallBatchNumRows = ParallelMergeCombiningSequence.DEFAULT_TASK_SMALL_BATCH_NUM_ROWS; } else { - warnDeprecated( + throw invalidConfigException( "druid.processing.merge.task.smallBatchNumRows", "druid.processing.merge.smallBatchNumRows" ); - this.smallBatchNumRows = oldConfig.getMergePoolSmallBatchRows(); } } else { this.smallBatchNumRows = smallBatchNumRows; @@ -164,7 +168,7 @@ public BrokerParallelMergeConfig( @VisibleForTesting public BrokerParallelMergeConfig() { - this(null, null, null, null, null, null, null, null); + this(null, null, null, null, null, null, null, null, null); } public boolean useParallelMergePool() @@ -202,13 +206,11 @@ public int getSmallBatchNumRows() return smallBatchNumRows; } - private static void warnDeprecated(String oldPath, String newPath) + private static DruidException invalidConfigException(String oldPath, String newPath) { - LOG.warn( - "Using deprecated config [%s] which has been replace by [%s]. This path is deprecated and will be " - + "removed in a future release, please transition to using [%s]", + return InvalidInput.exception( + "Config[%s] has been removed. Please use config[%s] instead.", oldPath, - newPath, newPath ); } diff --git a/server/src/main/java/org/apache/druid/query/LegacyBrokerParallelMergeConfig.java b/server/src/main/java/org/apache/druid/query/LegacyBrokerParallelMergeConfig.java deleted file mode 100644 index 25b11ab63d17..000000000000 --- a/server/src/main/java/org/apache/druid/query/LegacyBrokerParallelMergeConfig.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * 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.query; - -import org.skife.config.Config; - -import javax.annotation.Nullable; - -/** - * Backwards compatibility for Druid 27 and older runtime.properties configs, replaced by - * {@link BrokerParallelMergeConfig} in Druid 28. This config should be removed in Druid 29. - */ -@Deprecated -public abstract class LegacyBrokerParallelMergeConfig -{ - @Nullable - @Config(value = "druid.processing.merge.pool.parallelism") - public Integer getMergePoolParallelism() - { - return null; - } - - @Nullable - @Config(value = "druid.processing.merge.pool.awaitShutdownMillis") - public Long getMergePoolAwaitShutdownMillis() - { - return null; - } - - @Nullable - @Config(value = "druid.processing.merge.pool.defaultMaxQueryParallelism") - public Integer getMergePoolDefaultMaxQueryParallelism() - { - return null; - } - - @Nullable - @Config(value = "druid.processing.merge.task.targetRunTimeMillis") - public Integer getMergePoolTargetTaskRunTimeMillis() - { - return null; - } - - @Nullable - @Config(value = "druid.processing.merge.task.initialYieldNumRows") - public Integer getMergePoolTaskInitialYieldRows() - { - return null; - } - - @Nullable - @Config(value = "druid.processing.merge.task.smallBatchNumRows") - public Integer getMergePoolSmallBatchRows() - { - return null; - } -} diff --git a/server/src/main/java/org/apache/druid/query/LegacyBrokerPoolMergeConfig.java b/server/src/main/java/org/apache/druid/query/LegacyBrokerPoolMergeConfig.java new file mode 100644 index 000000000000..3143c9f10c05 --- /dev/null +++ b/server/src/main/java/org/apache/druid/query/LegacyBrokerPoolMergeConfig.java @@ -0,0 +1,67 @@ +/* + * 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.query; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +import javax.annotation.Nullable; + +/** + * @deprecated These config paths were removed in Druid 33 and this class has + * been retained only to generate appropriate error messages. + */ +@Deprecated +public class LegacyBrokerPoolMergeConfig +{ + @JsonProperty + private final Integer mergePoolParallelism; + @JsonProperty + private final Long mergePoolAwaitShutdownMillis; + @JsonProperty + private final Integer mergePoolDefaultMaxQueryParallelism; + + @JsonCreator + public LegacyBrokerPoolMergeConfig( + @JsonProperty("parallelism") @Nullable Integer mergePoolParallelism, + @JsonProperty("awaitShutdownMillis") @Nullable Long mergePoolAwaitShutdownMillis, + @JsonProperty("defaultMaxQueryParallelism") @Nullable Integer mergePoolDefaultMaxQueryParallelism + ) + { + this.mergePoolParallelism = mergePoolParallelism; + this.mergePoolAwaitShutdownMillis = mergePoolAwaitShutdownMillis; + this.mergePoolDefaultMaxQueryParallelism = mergePoolDefaultMaxQueryParallelism; + } + + public Integer getMergePoolDefaultMaxQueryParallelism() + { + return mergePoolDefaultMaxQueryParallelism; + } + + public Integer getMergePoolParallelism() + { + return mergePoolParallelism; + } + + public Long getMergePoolAwaitShutdownMillis() + { + return mergePoolAwaitShutdownMillis; + } +} diff --git a/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java b/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java new file mode 100644 index 000000000000..5a5c1ae5f0b7 --- /dev/null +++ b/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java @@ -0,0 +1,64 @@ +/* + * 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.query; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * @deprecated These config paths were removed in Druid 33 and this class has + * been retained only to generate appropriate error messages. + */ +public class LegacyBrokerTaskMergeConfig +{ + @JsonProperty + private final Integer mergePoolTargetTaskRunTimeMillis; + @JsonProperty + private final Integer mergePoolTaskInitialYieldRows; + @JsonProperty + private final Integer mergePoolSmallBatchRows; + + @JsonCreator + public LegacyBrokerTaskMergeConfig( + @JsonProperty("smallBatchNumRows") Integer mergePoolSmallBatchRows, + @JsonProperty("initialYieldNumRows") Integer mergePoolTaskInitialYieldRows, + @JsonProperty("targetRunTimeMillis") Integer mergePoolTargetTaskRunTimeMillis + ) + { + this.mergePoolSmallBatchRows = mergePoolSmallBatchRows; + this.mergePoolTaskInitialYieldRows = mergePoolTaskInitialYieldRows; + this.mergePoolTargetTaskRunTimeMillis = mergePoolTargetTaskRunTimeMillis; + } + + public Integer getMergePoolTargetTaskRunTimeMillis() + { + return mergePoolTargetTaskRunTimeMillis; + } + + public Integer getMergePoolTaskInitialYieldRows() + { + return mergePoolTaskInitialYieldRows; + } + + public Integer getMergePoolSmallBatchRows() + { + return mergePoolSmallBatchRows; + } +} diff --git a/server/src/test/java/org/apache/druid/guice/BrokerProcessingModuleTest.java b/server/src/test/java/org/apache/druid/guice/BrokerProcessingModuleTest.java index 7fca81aaea30..292b8796731a 100644 --- a/server/src/test/java/org/apache/druid/guice/BrokerProcessingModuleTest.java +++ b/server/src/test/java/org/apache/druid/guice/BrokerProcessingModuleTest.java @@ -19,6 +19,7 @@ package org.apache.druid.guice; +import com.amazonaws.util.Throwables; import com.google.common.collect.ImmutableList; import com.google.inject.Injector; import com.google.inject.ProvisionException; @@ -27,12 +28,12 @@ import org.apache.druid.client.cache.CacheConfig; import org.apache.druid.client.cache.CachePopulator; import org.apache.druid.client.cache.CachePopulatorStats; +import org.apache.druid.error.DruidExceptionMatcher; import org.apache.druid.initialization.Initialization; -import org.apache.druid.java.util.common.config.Config; import org.apache.druid.query.BrokerParallelMergeConfig; import org.apache.druid.query.DruidProcessingConfig; -import org.apache.druid.query.LegacyBrokerParallelMergeConfig; import org.apache.druid.utils.JvmUtils; +import org.hamcrest.MatcherAssert; import org.junit.Assert; import org.junit.Assume; import org.junit.Before; @@ -40,7 +41,6 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; -import org.skife.config.ConfigurationObjectFactory; import java.util.Properties; @@ -88,15 +88,68 @@ public void testMergeProcessingPool() @Test public void testMergeProcessingPoolLegacyConfigs() { - Properties props = new Properties(); - props.put("druid.processing.merge.pool.parallelism", "10"); - props.put("druid.processing.merge.pool.defaultMaxQueryParallelism", "10"); - props.put("druid.processing.merge.task.targetRunTimeMillis", "1000"); - Injector gadget = makeInjector(props); - BrokerParallelMergeConfig config = gadget.getInstance(BrokerParallelMergeConfig.class); - Assert.assertEquals(10, config.getParallelism()); - Assert.assertEquals(10, config.getDefaultMaxQueryParallelism()); - Assert.assertEquals(1000, config.getTargetRunTimeMillis()); + verifyOldConfigThrowsException( + "druid.processing.merge.pool.parallelism", + "10", + "Config[druid.processing.merge.pool.parallelism] has been removed." + + " Please use config[druid.processing.merge.parallelism] instead." + ); + verifyOldConfigThrowsException( + "druid.processing.merge.pool.awaitShutdownMillis", + "1000", + "Config[druid.processing.merge.pool.awaitShutdownMillis] has been removed." + + " Please use config[druid.processing.merge.awaitShutdownMillis] instead." + ); + verifyOldConfigThrowsException( + "druid.processing.merge.pool.defaultMaxQueryParallelism", + "100", + "Config[druid.processing.merge.pool.defaultMaxQueryParallelism] has been removed." + + " Please use config[druid.processing.merge.defaultMaxQueryParallelism] instead." + ); + } + + @Test + public void testMergeProcessingTaskLegacyConfigs() + { + verifyOldConfigThrowsException( + "druid.processing.merge.task.targetRunTimeMillis", + "10", + "Config[druid.processing.merge.task.targetRunTimeMillis] has been removed." + + " Please use config[druid.processing.merge.targetRunTimeMillis] instead." + ); + verifyOldConfigThrowsException( + "druid.processing.merge.task.initialYieldNumRows", + "1000", + "Config[druid.processing.merge.task.initialYieldNumRows] has been removed." + + " Please use config[druid.processing.merge.initialYieldNumRows] instead." + ); + verifyOldConfigThrowsException( + "druid.processing.merge.task.smallBatchNumRows", + "100", + "Config[druid.processing.merge.task.smallBatchNumRows] has been removed." + + " Please use config[druid.processing.merge.smallBatchNumRows] instead." + ); + } + + private void verifyOldConfigThrowsException( + String removedProperty, + String dummyValue, + String expectedMessage + ) + { + final Properties props = new Properties(); + props.setProperty(removedProperty, dummyValue); + + final Injector gadget = makeInjector(props); + MatcherAssert.assertThat( + Throwables.getRootCause( + Assert.assertThrows( + ProvisionException.class, + () -> gadget.getInstance(BrokerParallelMergeConfig.class) + ) + ), + DruidExceptionMatcher.invalidInput().expectMessageIs(expectedMessage) + ); } @Test @@ -138,10 +191,6 @@ private Injector makeInjector(Properties props) binder.bindConstant().annotatedWith(Names.named("servicePort")).to(0); binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(-1); binder.bind(Properties.class).toInstance(props); - ConfigurationObjectFactory factory = Config.createFactory(props); - LegacyBrokerParallelMergeConfig legacyConfig = factory.build(LegacyBrokerParallelMergeConfig.class); - binder.bind(ConfigurationObjectFactory.class).toInstance(factory); - binder.bind(LegacyBrokerParallelMergeConfig.class).toInstance(legacyConfig); }, target ).with((binder) -> { diff --git a/server/src/test/java/org/apache/druid/guice/QueryableModuleTest.java b/server/src/test/java/org/apache/druid/guice/QueryableModuleTest.java index 8fad8924bf88..bc74551d60a3 100644 --- a/server/src/test/java/org/apache/druid/guice/QueryableModuleTest.java +++ b/server/src/test/java/org/apache/druid/guice/QueryableModuleTest.java @@ -77,7 +77,6 @@ private Injector makeInjector(Properties properties) new JacksonModule(), new ConfigModule(), new QueryRunnerFactoryModule(), - new LegacyBrokerParallelMergeConfigModule(), new BrokerProcessingModule(), new LifecycleModule(), binder -> binder.bind(ServiceEmitter.class).to(NoopServiceEmitter.class), diff --git a/services/src/main/java/org/apache/druid/cli/CliBroker.java b/services/src/main/java/org/apache/druid/cli/CliBroker.java index 09212ec75972..42bb7b51421c 100644 --- a/services/src/main/java/org/apache/druid/cli/CliBroker.java +++ b/services/src/main/java/org/apache/druid/cli/CliBroker.java @@ -47,7 +47,6 @@ import org.apache.druid.guice.JoinableFactoryModule; import org.apache.druid.guice.JsonConfigProvider; import org.apache.druid.guice.LazySingleton; -import org.apache.druid.guice.LegacyBrokerParallelMergeConfigModule; import org.apache.druid.guice.LifecycleModule; import org.apache.druid.guice.ManageLifecycle; import org.apache.druid.guice.QueryRunnerFactoryModule; @@ -117,7 +116,6 @@ protected Set getNodeRoles(Properties properties) protected List getModules() { return ImmutableList.of( - new LegacyBrokerParallelMergeConfigModule(), new BrokerProcessingModule(), new QueryableModule(), new QueryRunnerFactoryModule(), From b2fa7281aae5e2abe5e5ca0f49ca9620990dde1b Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Wed, 5 Mar 2025 09:04:28 +0530 Subject: [PATCH 2/5] Add deprecated annotation --- .../java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java b/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java index 5a5c1ae5f0b7..708f24dcd5f2 100644 --- a/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java +++ b/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java @@ -26,6 +26,7 @@ * @deprecated These config paths were removed in Druid 33 and this class has * been retained only to generate appropriate error messages. */ +@Deprecated public class LegacyBrokerTaskMergeConfig { @JsonProperty From b59222a80927446873768a8871ba991fedcd57f9 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Wed, 5 Mar 2025 17:22:55 +0530 Subject: [PATCH 3/5] Move old property validation to StartupInjectorBuilder --- .../druid/guice/StartupInjectorBuilder.java | 43 ++++++ .../guice/StartupInjectorBuilderTest.java | 61 +++++++++ .../query/BrokerParallelMergeConfig.java | 122 ++++-------------- .../query/LegacyBrokerPoolMergeConfig.java | 67 ---------- .../query/LegacyBrokerTaskMergeConfig.java | 65 ---------- .../guice/BrokerProcessingModuleTest.java | 70 ---------- 6 files changed, 131 insertions(+), 297 deletions(-) delete mode 100644 server/src/main/java/org/apache/druid/query/LegacyBrokerPoolMergeConfig.java delete mode 100644 server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java diff --git a/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java b/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java index bdf73993efac..32644741b6f1 100644 --- a/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java +++ b/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java @@ -24,6 +24,7 @@ import com.google.inject.Injector; import com.google.inject.util.Providers; import org.apache.druid.common.config.NullValueHandlingConfig; +import org.apache.druid.error.InvalidInput; import org.apache.druid.jackson.JacksonModule; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; @@ -165,6 +166,48 @@ public void validate() docsLink ); } + + validateRemovedProcessingConfigs(); + } + + private void validateRemovedProcessingConfigs() + { + checkOldConfigAndThrow( + "druid.processing.merge.task.initialYieldNumRows", + "druid.processing.merge.initialYieldNumRows" + ); + checkOldConfigAndThrow( + "druid.processing.merge.task.targetRunTimeMillis", + "druid.processing.merge.targetRunTimeMillis" + ); + checkOldConfigAndThrow( + "druid.processing.merge.task.smallBatchNumRows", + "druid.processing.merge.smallBatchNumRows" + ); + + checkOldConfigAndThrow( + "druid.processing.merge.pool.awaitShutdownMillis", + "druid.processing.merge.awaitShutdownMillis" + ); + checkOldConfigAndThrow( + "druid.processing.merge.pool.parallelism", + "druid.processing.merge.parallelism" + ); + checkOldConfigAndThrow( + "druid.processing.merge.pool.defaultMaxQueryParallelism", + "druid.processing.merge.defaultMaxQueryParallelism" + ); + } + + private void checkOldConfigAndThrow(String oldPath, String newPath) + { + if (properties.getProperty(oldPath) != null) { + throw InvalidInput.exception( + "Config[%s] has been removed. Please use config[%s] instead.", + oldPath, + newPath + ); + } } } diff --git a/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java b/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java index 892434defcee..566a5b5fda94 100644 --- a/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java +++ b/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java @@ -21,10 +21,13 @@ import com.google.inject.Injector; import org.apache.druid.common.config.NullValueHandlingConfig; +import org.apache.druid.error.DruidException; +import org.apache.druid.error.DruidExceptionMatcher; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.ExpressionProcessingConfig; import org.apache.druid.utils.RuntimeInfo; +import org.hamcrest.MatcherAssert; import org.junit.Assert; import org.junit.Test; @@ -194,4 +197,62 @@ public void testValidator() t.getMessage() ); } + + @Test + public void testOldBrokerProcessingConfigs_throwException() + { + verifyOldConfigThrowsException( + "druid.processing.merge.pool.parallelism", + "10", + "Config[druid.processing.merge.pool.parallelism] has been removed." + + " Please use config[druid.processing.merge.parallelism] instead." + ); + verifyOldConfigThrowsException( + "druid.processing.merge.pool.awaitShutdownMillis", + "1000", + "Config[druid.processing.merge.pool.awaitShutdownMillis] has been removed." + + " Please use config[druid.processing.merge.awaitShutdownMillis] instead." + ); + verifyOldConfigThrowsException( + "druid.processing.merge.pool.defaultMaxQueryParallelism", + "100", + "Config[druid.processing.merge.pool.defaultMaxQueryParallelism] has been removed." + + " Please use config[druid.processing.merge.defaultMaxQueryParallelism] instead." + ); + + verifyOldConfigThrowsException( + "druid.processing.merge.task.targetRunTimeMillis", + "10", + "Config[druid.processing.merge.task.targetRunTimeMillis] has been removed." + + " Please use config[druid.processing.merge.targetRunTimeMillis] instead." + ); + verifyOldConfigThrowsException( + "druid.processing.merge.task.initialYieldNumRows", + "1000", + "Config[druid.processing.merge.task.initialYieldNumRows] has been removed." + + " Please use config[druid.processing.merge.initialYieldNumRows] instead." + ); + verifyOldConfigThrowsException( + "druid.processing.merge.task.smallBatchNumRows", + "100", + "Config[druid.processing.merge.task.smallBatchNumRows] has been removed." + + " Please use config[druid.processing.merge.smallBatchNumRows] instead." + ); + } + + private static void verifyOldConfigThrowsException( + String removedProperty, + String dummyValue, + String expectedMessage + ) + { + final Properties props = new Properties(); + props.setProperty(removedProperty, dummyValue); + + final StartupInjectorBuilder builder = new StartupInjectorBuilder().withExtensions().withProperties(props); + MatcherAssert.assertThat( + Assert.assertThrows(DruidException.class, builder::build), + DruidExceptionMatcher.invalidInput().expectMessageIs(expectedMessage) + ); + } } diff --git a/server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java b/server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java index a41fd1310989..a7f0123c155a 100644 --- a/server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java +++ b/server/src/main/java/org/apache/druid/query/BrokerParallelMergeConfig.java @@ -22,8 +22,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.annotations.VisibleForTesting; -import org.apache.druid.error.DruidException; -import org.apache.druid.error.InvalidInput; +import org.apache.druid.common.config.Configs; import org.apache.druid.java.util.common.guava.ParallelMergeCombiningSequence; import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.utils.JvmUtils; @@ -50,12 +49,6 @@ public class BrokerParallelMergeConfig @JsonProperty private final int smallBatchNumRows; - // These are needed to make JsonConfigurator happy - @JsonProperty - private final LegacyBrokerTaskMergeConfig task; - @JsonProperty - private final LegacyBrokerPoolMergeConfig pool; - @JsonCreator public BrokerParallelMergeConfig( @JsonProperty("useParallelMergePool") @Nullable Boolean useParallelMergePool, @@ -64,27 +57,15 @@ public BrokerParallelMergeConfig( @JsonProperty("defaultMaxQueryParallelism") @Nullable Integer defaultMaxQueryParallelism, @JsonProperty("targetRunTimeMillis") @Nullable Integer targetRunTimeMillis, @JsonProperty("initialYieldNumRows") @Nullable Integer initialYieldNumRows, - @JsonProperty("smallBatchNumRows") @Nullable Integer smallBatchNumRows, - @JsonProperty("task") @Nullable LegacyBrokerTaskMergeConfig oldTaskConfig, - @JsonProperty("pool") @Nullable LegacyBrokerPoolMergeConfig oldPoolConfig + @JsonProperty("smallBatchNumRows") @Nullable Integer smallBatchNumRows ) { - this.task = oldTaskConfig; - this.pool = oldPoolConfig; - if (parallelism == null) { - if (oldPoolConfig == null || oldPoolConfig.getMergePoolParallelism() == null) { + this.parallelism = Configs.valueOrDefault( + parallelism, // assume 2 hyper-threads per core, so that this value is probably by default the number // of physical cores * 1.5 - this.parallelism = (int) Math.ceil(JvmUtils.getRuntimeInfo().getAvailableProcessors() * 0.75); - } else { - throw invalidConfigException( - "druid.processing.merge.pool.parallelism", - "druid.processing.merge.parallelism" - ); - } - } else { - this.parallelism = parallelism; - } + (int) Math.ceil(JvmUtils.getRuntimeInfo().getAvailableProcessors() * 0.75) + ); // need at least 3 to do 2 layer merge if (this.parallelism > 2) { @@ -99,76 +80,36 @@ public BrokerParallelMergeConfig( this.useParallelMergePool = false; } - if (awaitShutdownMillis == null) { - if (oldPoolConfig == null || oldPoolConfig.getMergePoolAwaitShutdownMillis() == null) { - this.awaitShutdownMillis = DEFAULT_MERGE_POOL_AWAIT_SHUTDOWN_MILLIS; - } else { - throw invalidConfigException( - "druid.processing.merge.pool.awaitShutdownMillis", - "druid.processing.merge.awaitShutdownMillis" - ); - } - } else { - this.awaitShutdownMillis = awaitShutdownMillis; - } + this.awaitShutdownMillis = Configs.valueOrDefault( + awaitShutdownMillis, + DEFAULT_MERGE_POOL_AWAIT_SHUTDOWN_MILLIS + ); - if (defaultMaxQueryParallelism == null) { - if (oldPoolConfig == null || oldPoolConfig.getMergePoolDefaultMaxQueryParallelism() == null) { - this.defaultMaxQueryParallelism = (int) Math.max(JvmUtils.getRuntimeInfo().getAvailableProcessors() * 0.5, 1); - } else { - throw invalidConfigException( - "druid.processing.merge.pool.defaultMaxQueryParallelism", - "druid.processing.merge.defaultMaxQueryParallelism" - ); - } - } else { - this.defaultMaxQueryParallelism = defaultMaxQueryParallelism; - } + this.defaultMaxQueryParallelism = Configs.valueOrDefault( + defaultMaxQueryParallelism, + (int) Math.max(JvmUtils.getRuntimeInfo().getAvailableProcessors() * 0.5, 1) + ); - if (targetRunTimeMillis == null) { - if (oldTaskConfig == null || oldTaskConfig.getMergePoolTargetTaskRunTimeMillis() == null) { - this.targetRunTimeMillis = ParallelMergeCombiningSequence.DEFAULT_TASK_TARGET_RUN_TIME_MILLIS; - } else { - throw invalidConfigException( - "druid.processing.merge.task.targetRunTimeMillis", - "druid.processing.merge.targetRunTimeMillis" - ); - } - } else { - this.targetRunTimeMillis = targetRunTimeMillis; - } + this.targetRunTimeMillis = Configs.valueOrDefault( + targetRunTimeMillis, + ParallelMergeCombiningSequence.DEFAULT_TASK_TARGET_RUN_TIME_MILLIS + ); - if (initialYieldNumRows == null) { - if (oldTaskConfig == null || oldTaskConfig.getMergePoolTaskInitialYieldRows() == null) { - this.initialYieldNumRows = ParallelMergeCombiningSequence.DEFAULT_TASK_INITIAL_YIELD_NUM_ROWS; - } else { - throw invalidConfigException( - "druid.processing.merge.task.initialYieldNumRows", - "druid.processing.merge.initialYieldNumRows" - ); - } - } else { - this.initialYieldNumRows = initialYieldNumRows; - } + this.initialYieldNumRows = Configs.valueOrDefault( + initialYieldNumRows, + ParallelMergeCombiningSequence.DEFAULT_TASK_INITIAL_YIELD_NUM_ROWS + ); - if (smallBatchNumRows == null) { - if (oldTaskConfig == null || oldTaskConfig.getMergePoolSmallBatchRows() == null) { - this.smallBatchNumRows = ParallelMergeCombiningSequence.DEFAULT_TASK_SMALL_BATCH_NUM_ROWS; - } else { - throw invalidConfigException( - "druid.processing.merge.task.smallBatchNumRows", - "druid.processing.merge.smallBatchNumRows" - ); - } - } else { - this.smallBatchNumRows = smallBatchNumRows; - } + this.smallBatchNumRows = Configs.valueOrDefault( + smallBatchNumRows, + ParallelMergeCombiningSequence.DEFAULT_TASK_SMALL_BATCH_NUM_ROWS + ); } @VisibleForTesting public BrokerParallelMergeConfig() { - this(null, null, null, null, null, null, null, null, null); + this(null, null, null, null, null, null, null); } public boolean useParallelMergePool() @@ -205,13 +146,4 @@ public int getSmallBatchNumRows() { return smallBatchNumRows; } - - private static DruidException invalidConfigException(String oldPath, String newPath) - { - return InvalidInput.exception( - "Config[%s] has been removed. Please use config[%s] instead.", - oldPath, - newPath - ); - } } diff --git a/server/src/main/java/org/apache/druid/query/LegacyBrokerPoolMergeConfig.java b/server/src/main/java/org/apache/druid/query/LegacyBrokerPoolMergeConfig.java deleted file mode 100644 index 3143c9f10c05..000000000000 --- a/server/src/main/java/org/apache/druid/query/LegacyBrokerPoolMergeConfig.java +++ /dev/null @@ -1,67 +0,0 @@ -/* - * 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.query; - -import com.fasterxml.jackson.annotation.JsonCreator; -import com.fasterxml.jackson.annotation.JsonProperty; - -import javax.annotation.Nullable; - -/** - * @deprecated These config paths were removed in Druid 33 and this class has - * been retained only to generate appropriate error messages. - */ -@Deprecated -public class LegacyBrokerPoolMergeConfig -{ - @JsonProperty - private final Integer mergePoolParallelism; - @JsonProperty - private final Long mergePoolAwaitShutdownMillis; - @JsonProperty - private final Integer mergePoolDefaultMaxQueryParallelism; - - @JsonCreator - public LegacyBrokerPoolMergeConfig( - @JsonProperty("parallelism") @Nullable Integer mergePoolParallelism, - @JsonProperty("awaitShutdownMillis") @Nullable Long mergePoolAwaitShutdownMillis, - @JsonProperty("defaultMaxQueryParallelism") @Nullable Integer mergePoolDefaultMaxQueryParallelism - ) - { - this.mergePoolParallelism = mergePoolParallelism; - this.mergePoolAwaitShutdownMillis = mergePoolAwaitShutdownMillis; - this.mergePoolDefaultMaxQueryParallelism = mergePoolDefaultMaxQueryParallelism; - } - - public Integer getMergePoolDefaultMaxQueryParallelism() - { - return mergePoolDefaultMaxQueryParallelism; - } - - public Integer getMergePoolParallelism() - { - return mergePoolParallelism; - } - - public Long getMergePoolAwaitShutdownMillis() - { - return mergePoolAwaitShutdownMillis; - } -} diff --git a/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java b/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java deleted file mode 100644 index 708f24dcd5f2..000000000000 --- a/server/src/main/java/org/apache/druid/query/LegacyBrokerTaskMergeConfig.java +++ /dev/null @@ -1,65 +0,0 @@ -/* - * 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.query; - -import com.fasterxml.jackson.annotation.JsonCreator; -import com.fasterxml.jackson.annotation.JsonProperty; - -/** - * @deprecated These config paths were removed in Druid 33 and this class has - * been retained only to generate appropriate error messages. - */ -@Deprecated -public class LegacyBrokerTaskMergeConfig -{ - @JsonProperty - private final Integer mergePoolTargetTaskRunTimeMillis; - @JsonProperty - private final Integer mergePoolTaskInitialYieldRows; - @JsonProperty - private final Integer mergePoolSmallBatchRows; - - @JsonCreator - public LegacyBrokerTaskMergeConfig( - @JsonProperty("smallBatchNumRows") Integer mergePoolSmallBatchRows, - @JsonProperty("initialYieldNumRows") Integer mergePoolTaskInitialYieldRows, - @JsonProperty("targetRunTimeMillis") Integer mergePoolTargetTaskRunTimeMillis - ) - { - this.mergePoolSmallBatchRows = mergePoolSmallBatchRows; - this.mergePoolTaskInitialYieldRows = mergePoolTaskInitialYieldRows; - this.mergePoolTargetTaskRunTimeMillis = mergePoolTargetTaskRunTimeMillis; - } - - public Integer getMergePoolTargetTaskRunTimeMillis() - { - return mergePoolTargetTaskRunTimeMillis; - } - - public Integer getMergePoolTaskInitialYieldRows() - { - return mergePoolTaskInitialYieldRows; - } - - public Integer getMergePoolSmallBatchRows() - { - return mergePoolSmallBatchRows; - } -} diff --git a/server/src/test/java/org/apache/druid/guice/BrokerProcessingModuleTest.java b/server/src/test/java/org/apache/druid/guice/BrokerProcessingModuleTest.java index 292b8796731a..a4bf2e282114 100644 --- a/server/src/test/java/org/apache/druid/guice/BrokerProcessingModuleTest.java +++ b/server/src/test/java/org/apache/druid/guice/BrokerProcessingModuleTest.java @@ -19,7 +19,6 @@ package org.apache.druid.guice; -import com.amazonaws.util.Throwables; import com.google.common.collect.ImmutableList; import com.google.inject.Injector; import com.google.inject.ProvisionException; @@ -28,12 +27,10 @@ import org.apache.druid.client.cache.CacheConfig; import org.apache.druid.client.cache.CachePopulator; import org.apache.druid.client.cache.CachePopulatorStats; -import org.apache.druid.error.DruidExceptionMatcher; import org.apache.druid.initialization.Initialization; import org.apache.druid.query.BrokerParallelMergeConfig; import org.apache.druid.query.DruidProcessingConfig; import org.apache.druid.utils.JvmUtils; -import org.hamcrest.MatcherAssert; import org.junit.Assert; import org.junit.Assume; import org.junit.Before; @@ -85,73 +82,6 @@ public void testMergeProcessingPool() module.getMergeProcessingPoolProvider(config); } - @Test - public void testMergeProcessingPoolLegacyConfigs() - { - verifyOldConfigThrowsException( - "druid.processing.merge.pool.parallelism", - "10", - "Config[druid.processing.merge.pool.parallelism] has been removed." - + " Please use config[druid.processing.merge.parallelism] instead." - ); - verifyOldConfigThrowsException( - "druid.processing.merge.pool.awaitShutdownMillis", - "1000", - "Config[druid.processing.merge.pool.awaitShutdownMillis] has been removed." - + " Please use config[druid.processing.merge.awaitShutdownMillis] instead." - ); - verifyOldConfigThrowsException( - "druid.processing.merge.pool.defaultMaxQueryParallelism", - "100", - "Config[druid.processing.merge.pool.defaultMaxQueryParallelism] has been removed." - + " Please use config[druid.processing.merge.defaultMaxQueryParallelism] instead." - ); - } - - @Test - public void testMergeProcessingTaskLegacyConfigs() - { - verifyOldConfigThrowsException( - "druid.processing.merge.task.targetRunTimeMillis", - "10", - "Config[druid.processing.merge.task.targetRunTimeMillis] has been removed." - + " Please use config[druid.processing.merge.targetRunTimeMillis] instead." - ); - verifyOldConfigThrowsException( - "druid.processing.merge.task.initialYieldNumRows", - "1000", - "Config[druid.processing.merge.task.initialYieldNumRows] has been removed." - + " Please use config[druid.processing.merge.initialYieldNumRows] instead." - ); - verifyOldConfigThrowsException( - "druid.processing.merge.task.smallBatchNumRows", - "100", - "Config[druid.processing.merge.task.smallBatchNumRows] has been removed." - + " Please use config[druid.processing.merge.smallBatchNumRows] instead." - ); - } - - private void verifyOldConfigThrowsException( - String removedProperty, - String dummyValue, - String expectedMessage - ) - { - final Properties props = new Properties(); - props.setProperty(removedProperty, dummyValue); - - final Injector gadget = makeInjector(props); - MatcherAssert.assertThat( - Throwables.getRootCause( - Assert.assertThrows( - ProvisionException.class, - () -> gadget.getInstance(BrokerParallelMergeConfig.class) - ) - ), - DruidExceptionMatcher.invalidInput().expectMessageIs(expectedMessage) - ); - } - @Test public void testCachePopulatorAsSingleton() { From fa6320ae4f34016fc6a34076b680f7b3e513c6ff Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Thu, 6 Mar 2025 09:33:31 +0530 Subject: [PATCH 4/5] Update docs, use ISE --- docs/configuration/index.md | 12 ++++----- docs/querying/query-context.md | 6 ++--- .../druid/guice/StartupInjectorBuilder.java | 26 ++++++++++--------- .../guice/StartupInjectorBuilderTest.java | 16 ++++++------ 4 files changed, 31 insertions(+), 29 deletions(-) diff --git a/docs/configuration/index.md b/docs/configuration/index.md index cd1b8dfc1f9e..0ed82017c3dd 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -1908,12 +1908,12 @@ The broker uses processing configs for nested groupBy queries. |`druid.processing.fifo`|If the processing queue should treat tasks of equal priority in a FIFO manner|`true`| |`druid.processing.tmpDir`|Path where temporary files created while processing a query should be stored. If specified, this configuration takes priority over the default `java.io.tmpdir` path.|path represented by `java.io.tmpdir`| |`druid.processing.merge.useParallelMergePool`|Enable automatic parallel merging for Brokers on a dedicated async ForkJoinPool. If `false`, instead merges will be done serially on the `HTTP` thread pool.|`true`| -|`druid.processing.merge.pool.parallelism`|Size of ForkJoinPool. Note that the default configuration assumes that the value returned by `Runtime.getRuntime().availableProcessors()` represents 2 hyper-threads per physical core, and multiplies this value by `0.75` in attempt to size `1.5` times the number of _physical_ cores.|`Runtime.getRuntime().availableProcessors() * 0.75` (rounded up)| -|`druid.processing.merge.pool.defaultMaxQueryParallelism`|Default maximum number of parallel merge tasks per query. Note that the default configuration assumes that the value returned by `Runtime.getRuntime().availableProcessors()` represents 2 hyper-threads per physical core, and multiplies this value by `0.5` in attempt to size to the number of _physical_ cores.|`Runtime.getRuntime().availableProcessors() * 0.5` (rounded up)| -|`druid.processing.merge.pool.awaitShutdownMillis`|Time to wait for merge ForkJoinPool tasks to complete before ungracefully stopping on process shutdown in milliseconds.|`60_000`| -|`druid.processing.merge.task.targetRunTimeMillis`|Ideal run-time of each ForkJoinPool merge task, before forking off a new task to continue merging sequences.|100| -|`druid.processing.merge.task.initialYieldNumRows`|Number of rows to yield per ForkJoinPool merge task, before forking off a new task to continue merging sequences.|16384| -|`druid.processing.merge.task.smallBatchNumRows`|Size of result batches to operate on in ForkJoinPool merge tasks.|4096| +|`druid.processing.merge.parallelism`|Size of ForkJoinPool. Note that the default configuration assumes that the value returned by `Runtime.getRuntime().availableProcessors()` represents 2 hyper-threads per physical core, and multiplies this value by `0.75` in attempt to size `1.5` times the number of _physical_ cores.|`Runtime.getRuntime().availableProcessors() * 0.75` (rounded up)| +|`druid.processing.merge.defaultMaxQueryParallelism`|Default maximum number of parallel merge tasks per query. Note that the default configuration assumes that the value returned by `Runtime.getRuntime().availableProcessors()` represents 2 hyper-threads per physical core, and multiplies this value by `0.5` in attempt to size to the number of _physical_ cores.|`Runtime.getRuntime().availableProcessors() * 0.5` (rounded up)| +|`druid.processing.merge.awaitShutdownMillis`|Time to wait for merge ForkJoinPool tasks to complete before ungracefully stopping on process shutdown in milliseconds.|`60_000`| +|`druid.processing.merge.targetRunTimeMillis`|Ideal run-time of each ForkJoinPool merge task, before forking off a new task to continue merging sequences.|100| +|`druid.processing.merge.initialYieldNumRows`|Number of rows to yield per ForkJoinPool merge task, before forking off a new task to continue merging sequences.|16384| +|`druid.processing.merge.smallBatchNumRows`|Size of result batches to operate on in ForkJoinPool merge tasks.|4096| The amount of direct memory needed by Druid is at least `druid.processing.buffer.sizeBytes * (druid.processing.numMergeBuffers + 1)`. You can diff --git a/docs/querying/query-context.md b/docs/querying/query-context.md index d5ddea04f27d..f62c7f8334f6 100644 --- a/docs/querying/query-context.md +++ b/docs/querying/query-context.md @@ -58,9 +58,9 @@ See [SQL query context](sql-query-context.md) for other query context parameters |`serializeDateTimeAsLong`| `false` | If true, DateTime is serialized as long in the result returned by Broker and the data transportation between Broker and compute process| |`serializeDateTimeAsLongInner`| `false` | If true, DateTime is serialized as long in the data transportation between Broker and compute process| |`enableParallelMerge`|`true`|Enable parallel result merging on the Broker. Note that `druid.processing.merge.useParallelMergePool` must be enabled for this setting to be set to `true`. See [Broker configuration](../configuration/index.md#broker) for more details.| -|`parallelMergeParallelism`|`druid.processing.merge.pool.parallelism`|Maximum number of parallel threads to use for parallel result merging on the Broker. See [Broker configuration](../configuration/index.md#broker) for more details.| -|`parallelMergeInitialYieldRows`|`druid.processing.merge.task.initialYieldNumRows`|Number of rows to yield per ForkJoinPool merge task for parallel result merging on the Broker, before forking off a new task to continue merging sequences. See [Broker configuration](../configuration/index.md#broker) for more details.| -|`parallelMergeSmallBatchRows`|`druid.processing.merge.task.smallBatchNumRows`|Size of result batches to operate on in ForkJoinPool merge tasks for parallel result merging on the Broker. See [Broker configuration](../configuration/index.md#broker) for more details.| +|`parallelMergeParallelism`|`druid.processing.merge.parallelism`|Maximum number of parallel threads to use for parallel result merging on the Broker. See [Broker configuration](../configuration/index.md#broker) for more details.| +|`parallelMergeInitialYieldRows`|`druid.processing.merge.initialYieldNumRows`|Number of rows to yield per ForkJoinPool merge task for parallel result merging on the Broker, before forking off a new task to continue merging sequences. See [Broker configuration](../configuration/index.md#broker) for more details.| +|`parallelMergeSmallBatchRows`|`druid.processing.merge.smallBatchNumRows`|Size of result batches to operate on in ForkJoinPool merge tasks for parallel result merging on the Broker. See [Broker configuration](../configuration/index.md#broker) for more details.| |`useFilterCNF`|`false`| If true, Druid will attempt to convert the query filter to Conjunctive Normal Form (CNF). During query processing, columns can be pre-filtered by intersecting the bitmap indexes of all values that match the eligible filters, often greatly reducing the raw number of rows which need to be scanned. But this effect only happens for the top level filter, or individual clauses of a top level 'and' filter. As such, filters in CNF potentially have a higher chance to utilize a large amount of bitmap indexes on string columns during pre-filtering. However, this setting should be used with great caution, as it can sometimes have a negative effect on performance, and in some cases, the act of computing CNF of a filter can be expensive. We recommend hand tuning your filters to produce an optimal form if possible, or at least verifying through experimentation that using this parameter actually improves your query performance with no ill-effects.| |`secondaryPartitionPruning`|`true`|Enable secondary partition pruning on the Broker. The Broker will always prune unnecessary segments from the input scan based on a filter on time intervals, but if the data is further partitioned with hash or range partitioning, this option will enable additional pruning based on a filter on secondary partition dimensions.| |`debug`| `false` | Flag indicating whether to enable debugging outputs for the query. When set to false, no additional logs will be produced (logs produced will be entirely dependent on your logging level). When set to true, the following addition logs will be produced:
- Log the stack trace of the exception (if any) produced by the query | diff --git a/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java b/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java index 32644741b6f1..1d524a9a1e79 100644 --- a/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java +++ b/processing/src/main/java/org/apache/druid/guice/StartupInjectorBuilder.java @@ -24,7 +24,6 @@ import com.google.inject.Injector; import com.google.inject.util.Providers; import org.apache.druid.common.config.NullValueHandlingConfig; -import org.apache.druid.error.InvalidInput; import org.apache.druid.jackson.JacksonModule; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; @@ -172,40 +171,43 @@ public void validate() private void validateRemovedProcessingConfigs() { - checkOldConfigAndThrow( + checkDeletedConfigAndThrow( "druid.processing.merge.task.initialYieldNumRows", "druid.processing.merge.initialYieldNumRows" ); - checkOldConfigAndThrow( + checkDeletedConfigAndThrow( "druid.processing.merge.task.targetRunTimeMillis", "druid.processing.merge.targetRunTimeMillis" ); - checkOldConfigAndThrow( + checkDeletedConfigAndThrow( "druid.processing.merge.task.smallBatchNumRows", "druid.processing.merge.smallBatchNumRows" ); - checkOldConfigAndThrow( + checkDeletedConfigAndThrow( "druid.processing.merge.pool.awaitShutdownMillis", "druid.processing.merge.awaitShutdownMillis" ); - checkOldConfigAndThrow( + checkDeletedConfigAndThrow( "druid.processing.merge.pool.parallelism", "druid.processing.merge.parallelism" ); - checkOldConfigAndThrow( + checkDeletedConfigAndThrow( "druid.processing.merge.pool.defaultMaxQueryParallelism", "druid.processing.merge.defaultMaxQueryParallelism" ); } - private void checkOldConfigAndThrow(String oldPath, String newPath) + /** + * Checks if a deleted config is present in the properties and throws an ISE. + */ + private void checkDeletedConfigAndThrow(String deletedConfigName, String replaceConfigName) { - if (properties.getProperty(oldPath) != null) { - throw InvalidInput.exception( + if (properties.getProperty(deletedConfigName) != null) { + throw new ISE( "Config[%s] has been removed. Please use config[%s] instead.", - oldPath, - newPath + deletedConfigName, + replaceConfigName ); } } diff --git a/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java b/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java index 566a5b5fda94..d9a70166a57d 100644 --- a/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java +++ b/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java @@ -199,40 +199,40 @@ public void testValidator() } @Test - public void testOldBrokerProcessingConfigs_throwException() + public void verifyInjectorBuild_withDeletedConfig_throwsException() { - verifyOldConfigThrowsException( + verifyInjectorBuild_withDeletedConfig_throwsException( "druid.processing.merge.pool.parallelism", "10", "Config[druid.processing.merge.pool.parallelism] has been removed." + " Please use config[druid.processing.merge.parallelism] instead." ); - verifyOldConfigThrowsException( + verifyInjectorBuild_withDeletedConfig_throwsException( "druid.processing.merge.pool.awaitShutdownMillis", "1000", "Config[druid.processing.merge.pool.awaitShutdownMillis] has been removed." + " Please use config[druid.processing.merge.awaitShutdownMillis] instead." ); - verifyOldConfigThrowsException( + verifyInjectorBuild_withDeletedConfig_throwsException( "druid.processing.merge.pool.defaultMaxQueryParallelism", "100", "Config[druid.processing.merge.pool.defaultMaxQueryParallelism] has been removed." + " Please use config[druid.processing.merge.defaultMaxQueryParallelism] instead." ); - verifyOldConfigThrowsException( + verifyInjectorBuild_withDeletedConfig_throwsException( "druid.processing.merge.task.targetRunTimeMillis", "10", "Config[druid.processing.merge.task.targetRunTimeMillis] has been removed." + " Please use config[druid.processing.merge.targetRunTimeMillis] instead." ); - verifyOldConfigThrowsException( + verifyInjectorBuild_withDeletedConfig_throwsException( "druid.processing.merge.task.initialYieldNumRows", "1000", "Config[druid.processing.merge.task.initialYieldNumRows] has been removed." + " Please use config[druid.processing.merge.initialYieldNumRows] instead." ); - verifyOldConfigThrowsException( + verifyInjectorBuild_withDeletedConfig_throwsException( "druid.processing.merge.task.smallBatchNumRows", "100", "Config[druid.processing.merge.task.smallBatchNumRows] has been removed." @@ -240,7 +240,7 @@ public void testOldBrokerProcessingConfigs_throwException() ); } - private static void verifyOldConfigThrowsException( + private static void verifyInjectorBuild_withDeletedConfig_throwsException( String removedProperty, String dummyValue, String expectedMessage From e59f7015d5af8c50f63b595504ca0561672723f5 Mon Sep 17 00:00:00 2001 From: Kashif Faraz Date: Thu, 6 Mar 2025 10:04:29 +0530 Subject: [PATCH 5/5] Update test --- .../org/apache/druid/guice/StartupInjectorBuilderTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java b/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java index d9a70166a57d..bcf85742c2ac 100644 --- a/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java +++ b/processing/src/test/java/org/apache/druid/guice/StartupInjectorBuilderTest.java @@ -21,8 +21,7 @@ import com.google.inject.Injector; import org.apache.druid.common.config.NullValueHandlingConfig; -import org.apache.druid.error.DruidException; -import org.apache.druid.error.DruidExceptionMatcher; +import org.apache.druid.error.ExceptionMatcher; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.math.expr.ExpressionProcessingConfig; @@ -251,8 +250,8 @@ private static void verifyInjectorBuild_withDeletedConfig_throwsException( final StartupInjectorBuilder builder = new StartupInjectorBuilder().withExtensions().withProperties(props); MatcherAssert.assertThat( - Assert.assertThrows(DruidException.class, builder::build), - DruidExceptionMatcher.invalidInput().expectMessageIs(expectedMessage) + Assert.assertThrows(ISE.class, builder::build), + ExceptionMatcher.of(ISE.class).expectMessageIs(expectedMessage) ); } }