From 57c023b4f9af7d8190cbe918fecb7623b5145283 Mon Sep 17 00:00:00 2001 From: imply-cheddar <86940447+imply-cheddar@users.noreply.github.com> Date: Mon, 16 Dec 2024 16:28:03 -0800 Subject: [PATCH 1/9] Move Guice to 5.1.0 and fix tests --- benchmarks/pom.xml | 5 - .../aliyun-oss-extensions/pom.xml | 5 - extensions-contrib/cassandra-storage/pom.xml | 5 - .../cloudfiles-extensions/pom.xml | 6 - extensions-contrib/grpc-query/pom.xml | 5 - .../kubernetes-overlord-extensions/pom.xml | 5 - .../materialized-view-selection/pom.xml | 5 - .../moving-average-query/pom.xml | 5 - .../sqlserver-metadata-storage/pom.xml | 5 - extensions-core/azure-extensions/pom.xml | 5 - extensions-core/google-extensions/pom.xml | 5 - extensions-core/hdfs-storage/pom.xml | 6 +- extensions-core/kubernetes-extensions/pom.xml | 5 - extensions-core/lookups-cached-global/pom.xml | 5 - extensions-core/multi-stage-query/pom.xml | 5 - .../mysql-metadata-storage/pom.xml | 5 - .../postgresql-metadata-storage/pom.xml | 6 +- extensions-core/s3-extensions/pom.xml | 5 - indexing-service/pom.xml | 6 +- integration-tests-ex/cases/pom.xml | 5 - integration-tests/pom.xml | 6 +- licenses.yaml | 6 +- pom.xml | 7 +- processing/pom.xml | 6 +- .../util/emitter/service/ServiceEmitter.java | 4 +- .../apache/druid/error/ExceptionMatcher.java | 107 ++++++++++++++++++ .../org/apache/druid/guice/PolyBindTest.java | 40 +++---- .../query/DruidProcessingConfigTest.java | 14 +-- quidem-ut/pom.xml | 4 - server/pom.xml | 6 +- .../druid/guice/DruidInjectorBuilder.java | 16 +++ .../druid/guice/DruidInjectorBuilderTest.java | 51 +++++++++ .../druid/guice/StorageNodeModuleTest.java | 91 ++++++++------- .../druid/server/QuerySchedulerTest.java | 37 +++--- services/pom.xml | 6 +- sql/pom.xml | 6 +- .../sql/calcite/util/SqlTestFramework.java | 14 ++- 37 files changed, 287 insertions(+), 238 deletions(-) create mode 100644 processing/src/test/java/org/apache/druid/error/ExceptionMatcher.java diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index bd8c89b4d7f6..c578cfb10bef 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -61,11 +61,6 @@ org.easymock easymock - - com.google.inject.extensions - guice-multibindings - provided - org.apache.druid druid-processing diff --git a/extensions-contrib/aliyun-oss-extensions/pom.xml b/extensions-contrib/aliyun-oss-extensions/pom.xml index 82ee50eddf97..77c76c28806a 100644 --- a/extensions-contrib/aliyun-oss-extensions/pom.xml +++ b/extensions-contrib/aliyun-oss-extensions/pom.xml @@ -80,11 +80,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - com.google.guava guava diff --git a/extensions-contrib/cassandra-storage/pom.xml b/extensions-contrib/cassandra-storage/pom.xml index 7b5fc2be1831..5a2504bd825c 100644 --- a/extensions-contrib/cassandra-storage/pom.xml +++ b/extensions-contrib/cassandra-storage/pom.xml @@ -174,11 +174,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - diff --git a/extensions-contrib/cloudfiles-extensions/pom.xml b/extensions-contrib/cloudfiles-extensions/pom.xml index 0fd5018d1311..e5076e1d2284 100644 --- a/extensions-contrib/cloudfiles-extensions/pom.xml +++ b/extensions-contrib/cloudfiles-extensions/pom.xml @@ -56,12 +56,6 @@ - - com.google.inject.extensions - guice-multibindings - provided - - commons-io commons-io diff --git a/extensions-contrib/grpc-query/pom.xml b/extensions-contrib/grpc-query/pom.xml index e46f0c35fd2d..0eb714419373 100644 --- a/extensions-contrib/grpc-query/pom.xml +++ b/extensions-contrib/grpc-query/pom.xml @@ -96,11 +96,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - com.google.guava guava diff --git a/extensions-contrib/kubernetes-overlord-extensions/pom.xml b/extensions-contrib/kubernetes-overlord-extensions/pom.xml index 55f970fb9c39..1d63a351c598 100644 --- a/extensions-contrib/kubernetes-overlord-extensions/pom.xml +++ b/extensions-contrib/kubernetes-overlord-extensions/pom.xml @@ -212,11 +212,6 @@ jackson-databind provided - - com.google.inject.extensions - guice-multibindings - provided - joda-time joda-time diff --git a/extensions-contrib/materialized-view-selection/pom.xml b/extensions-contrib/materialized-view-selection/pom.xml index 4e44bfde5c6c..1cc85426d2b6 100644 --- a/extensions-contrib/materialized-view-selection/pom.xml +++ b/extensions-contrib/materialized-view-selection/pom.xml @@ -84,11 +84,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - org.jdbi jdbi diff --git a/extensions-contrib/moving-average-query/pom.xml b/extensions-contrib/moving-average-query/pom.xml index 119296634566..d948bec36867 100644 --- a/extensions-contrib/moving-average-query/pom.xml +++ b/extensions-contrib/moving-average-query/pom.xml @@ -79,11 +79,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - com.google.guava guava diff --git a/extensions-contrib/sqlserver-metadata-storage/pom.xml b/extensions-contrib/sqlserver-metadata-storage/pom.xml index 1fd186f9dc84..37e39c3f5eae 100644 --- a/extensions-contrib/sqlserver-metadata-storage/pom.xml +++ b/extensions-contrib/sqlserver-metadata-storage/pom.xml @@ -67,11 +67,6 @@ jackson-databind provided - - com.google.inject.extensions - guice-multibindings - provided - org.apache.commons commons-dbcp2 diff --git a/extensions-core/azure-extensions/pom.xml b/extensions-core/azure-extensions/pom.xml index af36456bbe70..990913c0328d 100644 --- a/extensions-core/azure-extensions/pom.xml +++ b/extensions-core/azure-extensions/pom.xml @@ -111,11 +111,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - com.google.guava guava diff --git a/extensions-core/google-extensions/pom.xml b/extensions-core/google-extensions/pom.xml index 39f89a2e5a53..e83a85f55535 100644 --- a/extensions-core/google-extensions/pom.xml +++ b/extensions-core/google-extensions/pom.xml @@ -82,11 +82,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - com.google.guava guava diff --git a/extensions-core/hdfs-storage/pom.xml b/extensions-core/hdfs-storage/pom.xml index ac8f7878e5d6..881ba1884716 100644 --- a/extensions-core/hdfs-storage/pom.xml +++ b/extensions-core/hdfs-storage/pom.xml @@ -88,11 +88,7 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - + org.apache.commons commons-lang3 diff --git a/extensions-core/kubernetes-extensions/pom.xml b/extensions-core/kubernetes-extensions/pom.xml index 6ef180795e25..e9ca2cae0e35 100644 --- a/extensions-core/kubernetes-extensions/pom.xml +++ b/extensions-core/kubernetes-extensions/pom.xml @@ -109,11 +109,6 @@ jackson-databind provided - - com.google.inject.extensions - guice-multibindings - provided - joda-time joda-time diff --git a/extensions-core/lookups-cached-global/pom.xml b/extensions-core/lookups-cached-global/pom.xml index 6fed0c9cb17b..287c6a0d89cf 100644 --- a/extensions-core/lookups-cached-global/pom.xml +++ b/extensions-core/lookups-cached-global/pom.xml @@ -84,11 +84,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - org.jdbi jdbi diff --git a/extensions-core/multi-stage-query/pom.xml b/extensions-core/multi-stage-query/pom.xml index e2a7252908df..f80c4edaef83 100644 --- a/extensions-core/multi-stage-query/pom.xml +++ b/extensions-core/multi-stage-query/pom.xml @@ -71,11 +71,6 @@ guice provided - - com.google.inject.extensions - guice-multibindings - provided - com.google.guava guava diff --git a/extensions-core/mysql-metadata-storage/pom.xml b/extensions-core/mysql-metadata-storage/pom.xml index 57f5ab4f12aa..103bb005f4ea 100644 --- a/extensions-core/mysql-metadata-storage/pom.xml +++ b/extensions-core/mysql-metadata-storage/pom.xml @@ -78,11 +78,6 @@ jackson-databind provided - - com.google.inject.extensions - guice-multibindings - provided - org.apache.commons commons-dbcp2 diff --git a/extensions-core/postgresql-metadata-storage/pom.xml b/extensions-core/postgresql-metadata-storage/pom.xml index 9c4159e36e81..fafd54869abb 100644 --- a/extensions-core/postgresql-metadata-storage/pom.xml +++ b/extensions-core/postgresql-metadata-storage/pom.xml @@ -76,11 +76,7 @@ jackson-databind provided - - com.google.inject.extensions - guice-multibindings - provided - + org.apache.commons commons-dbcp2 diff --git a/extensions-core/s3-extensions/pom.xml b/extensions-core/s3-extensions/pom.xml index cb9a03a5bae8..04df8b58f96f 100644 --- a/extensions-core/s3-extensions/pom.xml +++ b/extensions-core/s3-extensions/pom.xml @@ -87,11 +87,6 @@ jackson-core provided - - com.google.inject.extensions - guice-multibindings - provided - org.apache.commons commons-lang3 diff --git a/indexing-service/pom.xml b/indexing-service/pom.xml index 412568ea88c9..dd28de400cc2 100644 --- a/indexing-service/pom.xml +++ b/indexing-service/pom.xml @@ -88,11 +88,7 @@ com.fasterxml.jackson.core jackson-databind - - com.google.inject.extensions - guice-multibindings - provided - + javax.ws.rs jsr311-api diff --git a/integration-tests-ex/cases/pom.xml b/integration-tests-ex/cases/pom.xml index 94c42a0555b4..f6c260603277 100644 --- a/integration-tests-ex/cases/pom.xml +++ b/integration-tests-ex/cases/pom.xml @@ -83,11 +83,6 @@ com.google.inject guice - - com.google.inject.extensions - guice-multibindings - provided - org.apache.curator curator-framework diff --git a/integration-tests/pom.xml b/integration-tests/pom.xml index be57e12bf3dc..411ba0a2d250 100644 --- a/integration-tests/pom.xml +++ b/integration-tests/pom.xml @@ -278,11 +278,7 @@ com.google.inject guice - - com.google.inject.extensions - guice-multibindings - provided - + com.fasterxml.jackson.core jackson-databind diff --git a/licenses.yaml b/licenses.yaml index 8044beb12bab..86ca3df93b65 100644 --- a/licenses.yaml +++ b/licenses.yaml @@ -371,19 +371,15 @@ name: Guice license_category: binary module: java-core license_name: Apache License version 2.0 -version: 4.2.2 +version: 5.1.0 libraries: - com.google.inject: guice - - com.google.inject.extensions: guice-multibindings - com.google.inject.extensions: guice-servlet - com.google.inject.extensions: guice-assistedinject notices: - guice: | Google Guice - Core Library Copyright 2006-2016 Google, Inc. - - guice-multibindings: | - Google Guice - Extensions - MultiBindings - Copyright 2006-2016 Google, Inc. - guice-servlet: | Google Guice - Extensions - Servlet Copyright 2006-2016 Google, Inc. diff --git a/pom.xml b/pom.xml index 3704542d0ce5..b5d2a25cd662 100644 --- a/pom.xml +++ b/pom.xml @@ -95,7 +95,7 @@ 2.35.1 8.5.4 32.0.1-jre - 4.2.2 + 5.1.0 1.3 9.4.56.v20240826 1.19.4 @@ -596,11 +596,6 @@ guice-servlet ${guice.version} - - com.google.inject.extensions - guice-multibindings - ${guice.version} - com.google.inject.extensions guice-assistedinject diff --git a/processing/pom.xml b/processing/pom.xml index efcaf62c4b71..c3f758f32139 100644 --- a/processing/pom.xml +++ b/processing/pom.xml @@ -103,11 +103,7 @@ com.google.inject guice - - com.google.inject.extensions - guice-multibindings - provided - + com.google.code.findbugs jsr305 diff --git a/processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceEmitter.java b/processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceEmitter.java index 17b3ac59db67..13593fd146fc 100644 --- a/processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceEmitter.java +++ b/processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceEmitter.java @@ -47,8 +47,8 @@ public ServiceEmitter( { this.serviceDimensions = ImmutableMap .builder() - .put("service", Preconditions.checkNotNull(service)) - .put("host", Preconditions.checkNotNull(host)) + .put("service", Preconditions.checkNotNull(service, "service should be non-null")) + .put("host", Preconditions.checkNotNull(host, "host should be non-null")) .putAll(otherServiceDimensions) .build(); this.emitter = emitter; diff --git a/processing/src/test/java/org/apache/druid/error/ExceptionMatcher.java b/processing/src/test/java/org/apache/druid/error/ExceptionMatcher.java new file mode 100644 index 000000000000..de9b2a9728cc --- /dev/null +++ b/processing/src/test/java/org/apache/druid/error/ExceptionMatcher.java @@ -0,0 +1,107 @@ +/* + * 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.error; + +import org.apache.druid.matchers.DruidMatchers; +import org.hamcrest.Description; +import org.hamcrest.DiagnosingMatcher; +import org.hamcrest.Matcher; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.hamcrest.core.AllOf; + +import java.util.ArrayList; + +public class ExceptionMatcher extends DiagnosingMatcher +{ + + public static ExceptionMatcher of(Class clazz) + { + return new ExceptionMatcher(clazz); + } + + private final AllOf delegate; + private final ArrayList> matcherList; + private final Class clazz; + + public ExceptionMatcher(Class clazz) + { + this.clazz = clazz; + + matcherList = new ArrayList<>(); + delegate = new AllOf<>(matcherList); + } + + public ExceptionMatcher expectMessageIs(String s) + { + return expectMessage(Matchers.equalTo(s)); + } + + public ExceptionMatcher expectMessageContains(String contains) + { + return expectMessage(Matchers.containsString(contains)); + } + + public ExceptionMatcher expectMessage(Matcher messageMatcher) + { + matcherList.add(0, DruidMatchers.fn("message", Throwable::getMessage, messageMatcher)); + return this; + } + + public ExceptionMatcher expectCause(Matcher causeMatcher) + { + matcherList.add(0, DruidMatchers.fn("cause", Throwable::getCause, causeMatcher)); + return this; + } + + @Override + protected boolean matches(Object item, Description mismatchDescription) + { + return delegate.matches(item, mismatchDescription); + } + + @Override + public void describeTo(Description description) + { + delegate.describeTo(description); + } + + public void assertThrowsAndMatches(ThrowingSupplier fn) + { + boolean thrown = false; + try { + fn.get(); + } + catch (Throwable e) { + if (clazz.isInstance(e)) { + MatcherAssert.assertThat(e, this); + thrown = true; + } else { + throw new RuntimeException(e); + } + } + MatcherAssert.assertThat(thrown, Matchers.is(true)); + } + + public interface ThrowingSupplier + { + void get() throws Exception; + } +} diff --git a/processing/src/test/java/org/apache/druid/guice/PolyBindTest.java b/processing/src/test/java/org/apache/druid/guice/PolyBindTest.java index 372d428458d6..41b46b9a7094 100644 --- a/processing/src/test/java/org/apache/druid/guice/PolyBindTest.java +++ b/processing/src/test/java/org/apache/druid/guice/PolyBindTest.java @@ -28,6 +28,7 @@ import com.google.inject.ProvisionException; import com.google.inject.multibindings.MapBinder; import com.google.inject.name.Names; +import org.apache.druid.error.ExceptionMatcher; import org.junit.Assert; import org.junit.Test; @@ -106,22 +107,16 @@ public void configure(Binder binder) Assert.assertEquals("B", injector.getInstance(Gogo.class).go()); Assert.assertEquals("A", injector.getInstance(Key.get(Gogo.class, Names.named("reverse"))).go()); props.setProperty("billy", "c"); - try { - Assert.assertEquals("A", injector.getInstance(Gogo.class).go()); - Assert.fail(); // should never be reached - } - catch (Exception e) { - Assert.assertTrue(e instanceof ProvisionException); - Assert.assertTrue(e.getMessage().contains("Unknown provider [c] of Key[type=org.apache.druid.guice.PolyBindTest$Gogo")); - } - try { - Assert.assertEquals("B", injector.getInstance(Key.get(Gogo.class, Names.named("reverse"))).go()); - Assert.fail(); // should never be reached - } - catch (Exception e) { - Assert.assertTrue(e instanceof ProvisionException); - Assert.assertTrue(e.getMessage().contains("Unknown provider [c] of Key[type=org.apache.druid.guice.PolyBindTest$Gogo")); - } + + ExceptionMatcher + .of(ProvisionException.class) + .expectMessageContains("Unknown provider [c] of Key[type=PolyBindTest$Gogo") + .assertThrowsAndMatches(() -> injector.getInstance(Gogo.class).go()); + + ExceptionMatcher + .of(ProvisionException.class) + .expectMessageContains("Unknown provider [c] of Key[type=PolyBindTest$Gogo") + .assertThrowsAndMatches(() -> injector.getInstance(Key.get(Gogo.class, Names.named("reverse"))).go()); // test default property value Assert.assertEquals("B", injector.getInstance(GogoSally.class).go()); @@ -130,14 +125,11 @@ public void configure(Binder binder) props.setProperty("sally", "b"); Assert.assertEquals("B", injector.getInstance(GogoSally.class).go()); props.setProperty("sally", "c"); - try { - injector.getInstance(GogoSally.class).go(); - Assert.fail(); // should never be reached - } - catch (Exception e) { - Assert.assertTrue(e instanceof ProvisionException); - Assert.assertTrue(e.getMessage().contains("Unknown provider [c] of Key[type=org.apache.druid.guice.PolyBindTest$GogoSally")); - } + + ExceptionMatcher + .of(ProvisionException.class) + .expectMessageContains("Unknown provider [c] of Key[type=PolyBindTest$GogoSally") + .assertThrowsAndMatches(() -> injector.getInstance(GogoSally.class).go()); } public interface Gogo diff --git a/processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java b/processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java index e19fbf755402..9f4aea66f368 100644 --- a/processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java +++ b/processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java @@ -21,6 +21,7 @@ import com.google.inject.Injector; import com.google.inject.ProvisionException; +import org.apache.druid.error.ExceptionMatcher; import org.apache.druid.guice.JsonConfigProvider; import org.apache.druid.guice.StartupInjectorBuilder; import org.apache.druid.utils.JvmUtils; @@ -133,14 +134,11 @@ public void testInvalidSizeBytes() HEAP_SIZE, props ); - Throwable t = Assert.assertThrows( - ProvisionException.class, - () -> injector.getInstance(DruidProcessingConfig.class) - ); - Assert.assertTrue( - t.getMessage() - .contains("Cannot construct instance of `org.apache.druid.java.util.common.HumanReadableBytes`, problem: Invalid format of number: -1. Negative value is not allowed.") - ); + + ExceptionMatcher.of(ProvisionException.class) + .expectMessageContains( + "Cannot construct instance of `HumanReadableBytes`, problem: Invalid format of number: -1. Negative value is not allowed." + ).assertThrowsAndMatches(() -> injector.getInstance(DruidProcessingConfig.class)); } @Test diff --git a/quidem-ut/pom.xml b/quidem-ut/pom.xml index dd88d7fc84fa..6912ec4f2f4e 100644 --- a/quidem-ut/pom.xml +++ b/quidem-ut/pom.xml @@ -262,10 +262,6 @@ com.google.inject guice - - com.google.inject.extensions - guice-multibindings - com.fasterxml.jackson.core jackson-databind diff --git a/server/pom.xml b/server/pom.xml index 45ca282f836f..3a411ce703b1 100644 --- a/server/pom.xml +++ b/server/pom.xml @@ -107,11 +107,7 @@ com.sun.jersey jersey-core - - com.google.inject.extensions - guice-multibindings - provided - + com.google.inject.extensions guice-servlet diff --git a/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java b/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java index 964acc31641e..c0886c63bf9b 100644 --- a/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java +++ b/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java @@ -24,6 +24,7 @@ import com.google.inject.Injector; import com.google.inject.Key; import com.google.inject.Module; +import com.google.inject.util.Modules; import org.apache.druid.discovery.NodeRole; import org.apache.druid.guice.annotations.Json; import org.apache.druid.guice.annotations.LoadScope; @@ -219,6 +220,21 @@ public List modules() return modules; } + public DruidInjectorBuilder overrideCurrentGuiceModules(List overrides) + { + for (Module override : overrides) { + if (override instanceof DruidModule) { + registerJacksonModules((DruidModule) override); + } + } + + final Module overriddenModule = Modules.override(modules).with(overrides); + modules.clear(); + modules.add(overriddenModule); + return this; + } + + public Injector build() { return Guice.createInjector(modules); diff --git a/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java b/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java index d55ebe8f16b1..dfa8c246d0f3 100644 --- a/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java +++ b/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java @@ -23,6 +23,7 @@ import com.fasterxml.jackson.annotation.JsonTypeName; import com.fasterxml.jackson.databind.Module; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.jsontype.NamedType; import com.fasterxml.jackson.databind.module.SimpleModule; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -35,6 +36,7 @@ import org.apache.druid.initialization.CoreInjectorBuilder; import org.apache.druid.initialization.DruidModule; import org.apache.druid.java.util.common.ISE; +import org.junit.Assert; import org.junit.Test; import javax.inject.Inject; @@ -59,6 +61,10 @@ public static class MockObjectExtension extends MockObject { } + public static class OverrideMockObjectExtension extends MockObject + { + } + public interface MockInterface { } @@ -235,6 +241,51 @@ public void testAddAll() throws IOException verifyInjector(injector); } + @Test + public void testOverrideAndUpdate() throws IOException + { + MockInterface mine = new MockComponent(); + + Injector injector = new CoreInjectorBuilder( + new StartupInjectorBuilder() + .forTests() + .withEmptyProperties() + .build() + ) + .addAll(Arrays.asList(new MockGuiceModule(), new MockDruidModule())) + .overrideCurrentGuiceModules(List.of( + new DruidModule() + { + @Override + public void configure(Binder binder) + { + binder.bind(MockInterface.class).toInstance(mine); + } + + @Override + public List getJacksonModules() + { + return List.of( + new SimpleModule("test-override") + .registerSubtypes( + new NamedType(OverrideMockObjectExtension.class, "override") + ) + ); + } + } + )) + .build(); + + verifyInjector(injector); + Assert.assertSame(mine, injector.getInstance(MockInterface.class)); + + // And that the Druid module set up Jackson. + String json = "{\"type\": \"override\"}"; + ObjectMapper om = injector.getInstance(Key.get(ObjectMapper.class, Json.class)); + MockObject obj = om.readValue(json, MockObject.class); + assertTrue(obj instanceof OverrideMockObjectExtension); + } + /** * Enable extensions. Then, exclude our JSON test module. As a result, the * JSON object will fail to deserialize. diff --git a/server/src/test/java/org/apache/druid/guice/StorageNodeModuleTest.java b/server/src/test/java/org/apache/druid/guice/StorageNodeModuleTest.java index 082361a1c885..ea3b6597730c 100644 --- a/server/src/test/java/org/apache/druid/guice/StorageNodeModuleTest.java +++ b/server/src/test/java/org/apache/druid/guice/StorageNodeModuleTest.java @@ -26,6 +26,7 @@ import com.google.inject.name.Names; import com.google.inject.util.Modules; import org.apache.druid.discovery.DataNodeService; +import org.apache.druid.error.ExceptionMatcher; import org.apache.druid.guice.annotations.Self; import org.apache.druid.initialization.Initialization; import org.apache.druid.query.DruidProcessingConfig; @@ -36,11 +37,8 @@ import org.apache.druid.server.coordination.ServerType; import org.junit.Assert; import org.junit.Before; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; -import org.mockito.Answers; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; @@ -52,12 +50,7 @@ public class StorageNodeModuleTest { private static final boolean INJECT_SERVER_TYPE_CONFIG = true; - @Rule - public ExpectedException exceptionRule = ExpectedException.none(); - - @Mock(answer = Answers.RETURNS_MOCKS) private DruidNode self; - @Mock(answer = Answers.RETURNS_MOCKS) private ServerTypeConfig serverTypeConfig; @Mock private DruidProcessingConfig druidProcessingConfig; @@ -66,22 +59,23 @@ public class StorageNodeModuleTest @Mock private StorageLocationConfig storageLocation; - private Injector injector; private StorageNodeModule target; @Before public void setUp() { + self = new DruidNode("test", "test-host", true, 80, 443, false, true); + serverTypeConfig = new ServerTypeConfig(ServerType.HISTORICAL); + Mockito.when(segmentLoaderConfig.getLocations()).thenReturn(Collections.singletonList(storageLocation)); target = new StorageNodeModule(); - injector = makeInjector(INJECT_SERVER_TYPE_CONFIG); } @Test public void testIsSegmentCacheConfiguredIsInjected() { - Boolean isSegmentCacheConfigured = injector.getInstance( + Boolean isSegmentCacheConfigured = injector().getInstance( Key.get(Boolean.class, Names.named(StorageNodeModule.IS_SEGMENT_CACHE_CONFIGURED)) ); Assert.assertNotNull(isSegmentCacheConfigured); @@ -92,7 +86,7 @@ public void testIsSegmentCacheConfiguredIsInjected() public void testIsSegmentCacheConfiguredWithNoLocationsConfiguredIsInjected() { mockSegmentCacheNotConfigured(); - Boolean isSegmentCacheConfigured = injector.getInstance( + Boolean isSegmentCacheConfigured = injector().getInstance( Key.get(Boolean.class, Names.named(StorageNodeModule.IS_SEGMENT_CACHE_CONFIGURED)) ); Assert.assertNotNull(isSegmentCacheConfigured); @@ -102,27 +96,33 @@ public void testIsSegmentCacheConfiguredWithNoLocationsConfiguredIsInjected() @Test public void getDataNodeServiceWithNoServerTypeConfigShouldThrowProvisionException() { - exceptionRule.expect(ProvisionException.class); - exceptionRule.expectMessage("Must override the binding for ServerTypeConfig if you want a DataNodeService."); - injector = makeInjector(!INJECT_SERVER_TYPE_CONFIG); - injector.getInstance(DataNodeService.class); + Injector injector = makeInjector(!INJECT_SERVER_TYPE_CONFIG); + + ExceptionMatcher + .of(ProvisionException.class) + .expectMessageContains("Must override the binding for ServerTypeConfig if you want a DataNodeService.") + .assertThrowsAndMatches(() -> injector.getInstance(DataNodeService.class)); } @Test public void getDataNodeServiceWithNoSegmentCacheConfiguredThrowProvisionException() { - exceptionRule.expect(ProvisionException.class); - exceptionRule.expectMessage("druid.segmentCache.locations must be set on historicals."); - Mockito.doReturn(ServerType.HISTORICAL).when(serverTypeConfig).getServerType(); mockSegmentCacheNotConfigured(); - injector.getInstance(DataNodeService.class); + + ExceptionMatcher + .of(ProvisionException.class) + .expectMessageContains("druid.segmentCache.locations must be set on historicals.") + .assertThrowsAndMatches(() -> injector().getInstance(DataNodeService.class)); } @Test public void getDataNodeServiceIsInjectedAsSingleton() { + final Injector injector = injector(); + DataNodeService dataNodeService = injector.getInstance(DataNodeService.class); Assert.assertNotNull(dataNodeService); + DataNodeService other = injector.getInstance(DataNodeService.class); Assert.assertSame(dataNodeService, other); } @@ -130,7 +130,7 @@ public void getDataNodeServiceIsInjectedAsSingleton() @Test public void getDataNodeServiceIsInjectedAndDiscoverable() { - DataNodeService dataNodeService = injector.getInstance(DataNodeService.class); + DataNodeService dataNodeService = injector().getInstance(DataNodeService.class); Assert.assertNotNull(dataNodeService); Assert.assertTrue(dataNodeService.isDiscoverable()); } @@ -139,7 +139,8 @@ public void getDataNodeServiceIsInjectedAndDiscoverable() public void getDataNodeServiceWithSegmentCacheNotConfiguredIsInjectedAndDiscoverable() { mockSegmentCacheNotConfigured(); - DataNodeService dataNodeService = injector.getInstance(DataNodeService.class); + serverTypeConfig = new ServerTypeConfig(ServerType.BROKER); + DataNodeService dataNodeService = injector().getInstance(DataNodeService.class); Assert.assertNotNull(dataNodeService); Assert.assertFalse(dataNodeService.isDiscoverable()); } @@ -147,8 +148,10 @@ public void getDataNodeServiceWithSegmentCacheNotConfiguredIsInjectedAndDiscover @Test public void testDruidServerMetadataIsInjectedAsSingleton() { + final Injector injector = injector(); DruidServerMetadata druidServerMetadata = injector.getInstance(DruidServerMetadata.class); Assert.assertNotNull(druidServerMetadata); + DruidServerMetadata other = injector.getInstance(DruidServerMetadata.class); Assert.assertSame(druidServerMetadata, other); } @@ -156,30 +159,38 @@ public void testDruidServerMetadataIsInjectedAsSingleton() @Test public void testDruidServerMetadataWithNoServerTypeConfigShouldThrowProvisionException() { - exceptionRule.expect(ProvisionException.class); - exceptionRule.expectMessage("Must override the binding for ServerTypeConfig if you want a DruidServerMetadata."); - injector = makeInjector(!INJECT_SERVER_TYPE_CONFIG); - injector.getInstance(DruidServerMetadata.class); + Injector injector = makeInjector(!INJECT_SERVER_TYPE_CONFIG); + + ExceptionMatcher + .of(ProvisionException.class) + .expectMessageContains("Must override the binding for ServerTypeConfig if you want a DruidServerMetadata.") + .assertThrowsAndMatches(() -> injector.getInstance(DruidServerMetadata.class)); + } + + private Injector injector() + { + return makeInjector(INJECT_SERVER_TYPE_CONFIG); } private Injector makeInjector(boolean withServerTypeConfig) { return Initialization.makeInjectorWithModules( GuiceInjectors.makeStartupInjector(), (ImmutableList.of(Modules.override( - (binder) -> { - binder.bind(DruidNode.class).annotatedWith(Self.class).toInstance(self); - binder.bindConstant().annotatedWith(Names.named("serviceName")).to("test"); - binder.bindConstant().annotatedWith(Names.named("servicePort")).to(0); - binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(-1); - binder.bind(DruidProcessingConfig.class).toInstance(druidProcessingConfig); - }, - target).with( - (binder) -> { - binder.bind(SegmentLoaderConfig.class).toInstance(segmentLoaderConfig); - if (withServerTypeConfig) { - binder.bind(ServerTypeConfig.class).toInstance(serverTypeConfig); - } - } + (binder) -> { + binder.bind(DruidNode.class).annotatedWith(Self.class).toInstance(self); + binder.bindConstant().annotatedWith(Names.named("serviceName")).to("test"); + binder.bindConstant().annotatedWith(Names.named("servicePort")).to(0); + binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(-1); + binder.bind(DruidProcessingConfig.class).toInstance(druidProcessingConfig); + }, + target + ).with( + (binder) -> { + binder.bind(SegmentLoaderConfig.class).toInstance(segmentLoaderConfig); + if (withServerTypeConfig) { + binder.bind(ServerTypeConfig.class).toInstance(serverTypeConfig); + } + } ) ))); } diff --git a/server/src/test/java/org/apache/druid/server/QuerySchedulerTest.java b/server/src/test/java/org/apache/druid/server/QuerySchedulerTest.java index b670cd4dd787..d28778586442 100644 --- a/server/src/test/java/org/apache/druid/server/QuerySchedulerTest.java +++ b/server/src/test/java/org/apache/druid/server/QuerySchedulerTest.java @@ -31,6 +31,7 @@ import com.google.inject.Key; import com.google.inject.ProvisionException; import org.apache.druid.client.SegmentServerSelector; +import org.apache.druid.error.ExceptionMatcher; import org.apache.druid.guice.GuiceInjectors; import org.apache.druid.guice.JsonConfigProvider; import org.apache.druid.guice.JsonConfigurator; @@ -68,6 +69,7 @@ import org.apache.druid.server.scheduling.ManualQueryPrioritizationStrategy; import org.apache.druid.server.scheduling.NoQueryLaningStrategy; import org.easymock.EasyMock; +import org.hamcrest.text.StringContainsInOrder; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -492,16 +494,14 @@ public void testMisConfigHiLo() final Properties properties = new Properties(); properties.setProperty(propertyPrefix + ".laning.strategy", "hilo"); provider.inject(properties, injector.getInstance(JsonConfigurator.class)); - Throwable t = Assert.assertThrows(ProvisionException.class, () -> provider.get().get()); - Assert.assertEquals( - "Unable to provision, see the following errors:\n" - + "\n" - + "1) Problem parsing object at prefix[druid.query.scheduler]: Cannot construct instance of `org.apache.druid.server.scheduling.HiLoQueryLaningStrategy`, problem: maxLowPercent must be set\n" - + " at [Source: UNKNOWN; line: -1, column: -1] (through reference chain: org.apache.druid.server.QuerySchedulerProvider[\"laning\"]).\n" - + "\n" - + "1 error", - t.getMessage() - ); + ExceptionMatcher + .of(ProvisionException.class) + .expectMessage( + new StringContainsInOrder(List.of( + "Problem parsing object at prefix[druid.query.scheduler]:", + "problem: maxLowPercent must be set" + )) + ); } @Test @@ -550,15 +550,14 @@ public void testMisConfigThreshold() properties.setProperty(propertyPrefix + ".prioritization.strategy", "threshold"); provider.inject(properties, injector.getInstance(JsonConfigurator.class)); Throwable t = Assert.assertThrows(ProvisionException.class, () -> provider.get().get()); - Assert.assertEquals( - "Unable to provision, see the following errors:\n" - + "\n" - + "1) Problem parsing object at prefix[druid.query.scheduler]: Cannot construct instance of `org.apache.druid.server.scheduling.ThresholdBasedQueryPrioritizationStrategy`, problem: periodThreshold, durationThreshold, segmentCountThreshold or segmentRangeThreshold must be set\n" - + " at [Source: UNKNOWN; line: -1, column: -1] (through reference chain: org.apache.druid.server.QuerySchedulerProvider[\"prioritization\"]).\n" - + "\n" - + "1 error", - t.getMessage() - ); + ExceptionMatcher + .of(ProvisionException.class) + .expectMessage( + new StringContainsInOrder(List.of( + "Problem parsing object at prefix[druid.query.scheduler]:", + "problem: periodThreshold, durationThreshold, segmentCountThreshold or segmentRangeThreshold must be set" + )) + ); } diff --git a/services/pom.xml b/services/pom.xml index 7b681a1e7e17..5f46dd40f6d2 100644 --- a/services/pom.xml +++ b/services/pom.xml @@ -157,11 +157,7 @@ com.sun.jersey jersey-server - - com.google.inject.extensions - guice-multibindings - provided - + org.roaringbitmap RoaringBitmap diff --git a/sql/pom.xml b/sql/pom.xml index 1c68ad0c0ed5..32d3131588db 100644 --- a/sql/pom.xml +++ b/sql/pom.xml @@ -128,11 +128,7 @@ com.opencsv opencsv - - com.google.inject.extensions - guice-multibindings - provided - + javax.ws.rs jsr311-api diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java index a812f9ffa87d..cd8cab32ca55 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java @@ -36,7 +36,6 @@ import org.apache.druid.guice.StartupInjectorBuilder; import org.apache.druid.initialization.CoreInjectorBuilder; import org.apache.druid.initialization.DruidModule; -import org.apache.druid.initialization.ServiceInjectorBuilder; import org.apache.druid.java.util.common.RE; import org.apache.druid.java.util.common.io.Closer; import org.apache.druid.math.expr.ExprMacroTable; @@ -905,10 +904,17 @@ private SqlTestFramework(Builder builder) overrideModules.add(testSetupModule()); builder.componentSupplier.configureGuice(injectorBuilder, overrideModules); - ServiceInjectorBuilder serviceInjector = new ServiceInjectorBuilder(injectorBuilder); - serviceInjector.addAll(overrideModules); + // First override with the testSetupModule. It would be better if the existing tests were updated to not require + // this step of override as this is actually an indication that the objects aren't managing their bindings + // properly. This can cause obfuscation when trying to figure out where a binding/object is coming from, slowing + // developer productivity. For expediency, we add this extra layer of overrides just to make things work while + // work can then be done to fix the problematic configurations. It would be awesome if this override was no + // longer neede some day. + injectorBuilder.overrideCurrentGuiceModules(List.of(testSetupModule())); - this.injector = serviceInjector.build(); + injectorBuilder.overrideCurrentGuiceModules(overrideModules); + + this.injector = injectorBuilder.build(); this.engine = builder.componentSupplier.createEngine(queryLifecycleFactory(), queryJsonMapper(), injector); componentSupplier.configureJsonMapper(queryJsonMapper()); componentSupplier.finalizeTestFramework(this); From 3882f02a99fcabdb3cb8169d4ee7fdec94f001c8 Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Tue, 17 Dec 2024 14:37:53 +0530 Subject: [PATCH 2/9] Fix checkstyle --- .../druid/guice/StorageNodeModuleTest.java | 31 +++++++++---------- .../druid/server/QuerySchedulerTest.java | 4 +-- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/server/src/test/java/org/apache/druid/guice/StorageNodeModuleTest.java b/server/src/test/java/org/apache/druid/guice/StorageNodeModuleTest.java index ea3b6597730c..0f63572ed731 100644 --- a/server/src/test/java/org/apache/druid/guice/StorageNodeModuleTest.java +++ b/server/src/test/java/org/apache/druid/guice/StorageNodeModuleTest.java @@ -176,22 +176,21 @@ private Injector makeInjector(boolean withServerTypeConfig) { return Initialization.makeInjectorWithModules( GuiceInjectors.makeStartupInjector(), (ImmutableList.of(Modules.override( - (binder) -> { - binder.bind(DruidNode.class).annotatedWith(Self.class).toInstance(self); - binder.bindConstant().annotatedWith(Names.named("serviceName")).to("test"); - binder.bindConstant().annotatedWith(Names.named("servicePort")).to(0); - binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(-1); - binder.bind(DruidProcessingConfig.class).toInstance(druidProcessingConfig); - }, - target - ).with( - (binder) -> { - binder.bind(SegmentLoaderConfig.class).toInstance(segmentLoaderConfig); - if (withServerTypeConfig) { - binder.bind(ServerTypeConfig.class).toInstance(serverTypeConfig); - } - } - ) + (binder) -> { + binder.bind(DruidNode.class).annotatedWith(Self.class).toInstance(self); + binder.bindConstant().annotatedWith(Names.named("serviceName")).to("test"); + binder.bindConstant().annotatedWith(Names.named("servicePort")).to(0); + binder.bindConstant().annotatedWith(Names.named("tlsServicePort")).to(-1); + binder.bind(DruidProcessingConfig.class).toInstance(druidProcessingConfig); + }, + target).with( + (binder) -> { + binder.bind(SegmentLoaderConfig.class).toInstance(segmentLoaderConfig); + if (withServerTypeConfig) { + binder.bind(ServerTypeConfig.class).toInstance(serverTypeConfig); + } + } + ) ))); } diff --git a/server/src/test/java/org/apache/druid/server/QuerySchedulerTest.java b/server/src/test/java/org/apache/druid/server/QuerySchedulerTest.java index d28778586442..f5c26c01f241 100644 --- a/server/src/test/java/org/apache/druid/server/QuerySchedulerTest.java +++ b/server/src/test/java/org/apache/druid/server/QuerySchedulerTest.java @@ -501,7 +501,7 @@ public void testMisConfigHiLo() "Problem parsing object at prefix[druid.query.scheduler]:", "problem: maxLowPercent must be set" )) - ); + ); } @Test @@ -557,7 +557,7 @@ public void testMisConfigThreshold() "Problem parsing object at prefix[druid.query.scheduler]:", "problem: periodThreshold, durationThreshold, segmentCountThreshold or segmentRangeThreshold must be set" )) - ); + ); } From 4478f40d4a8ce2ab280c246f58e320255a903830 Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Tue, 17 Dec 2024 15:12:09 +0530 Subject: [PATCH 3/9] Revert overrideCurrentGuiceModules() and related changes --- .../druid/guice/DruidInjectorBuilder.java | 15 ------ .../druid/guice/DruidInjectorBuilderTest.java | 49 ------------------- .../sql/calcite/util/SqlTestFramework.java | 12 ++--- 3 files changed, 3 insertions(+), 73 deletions(-) diff --git a/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java b/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java index c0886c63bf9b..ddf8b9096f50 100644 --- a/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java +++ b/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java @@ -24,7 +24,6 @@ import com.google.inject.Injector; import com.google.inject.Key; import com.google.inject.Module; -import com.google.inject.util.Modules; import org.apache.druid.discovery.NodeRole; import org.apache.druid.guice.annotations.Json; import org.apache.druid.guice.annotations.LoadScope; @@ -220,20 +219,6 @@ public List modules() return modules; } - public DruidInjectorBuilder overrideCurrentGuiceModules(List overrides) - { - for (Module override : overrides) { - if (override instanceof DruidModule) { - registerJacksonModules((DruidModule) override); - } - } - - final Module overriddenModule = Modules.override(modules).with(overrides); - modules.clear(); - modules.add(overriddenModule); - return this; - } - public Injector build() { diff --git a/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java b/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java index dfa8c246d0f3..050693268541 100644 --- a/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java +++ b/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java @@ -61,10 +61,6 @@ public static class MockObjectExtension extends MockObject { } - public static class OverrideMockObjectExtension extends MockObject - { - } - public interface MockInterface { } @@ -241,51 +237,6 @@ public void testAddAll() throws IOException verifyInjector(injector); } - @Test - public void testOverrideAndUpdate() throws IOException - { - MockInterface mine = new MockComponent(); - - Injector injector = new CoreInjectorBuilder( - new StartupInjectorBuilder() - .forTests() - .withEmptyProperties() - .build() - ) - .addAll(Arrays.asList(new MockGuiceModule(), new MockDruidModule())) - .overrideCurrentGuiceModules(List.of( - new DruidModule() - { - @Override - public void configure(Binder binder) - { - binder.bind(MockInterface.class).toInstance(mine); - } - - @Override - public List getJacksonModules() - { - return List.of( - new SimpleModule("test-override") - .registerSubtypes( - new NamedType(OverrideMockObjectExtension.class, "override") - ) - ); - } - } - )) - .build(); - - verifyInjector(injector); - Assert.assertSame(mine, injector.getInstance(MockInterface.class)); - - // And that the Druid module set up Jackson. - String json = "{\"type\": \"override\"}"; - ObjectMapper om = injector.getInstance(Key.get(ObjectMapper.class, Json.class)); - MockObject obj = om.readValue(json, MockObject.class); - assertTrue(obj instanceof OverrideMockObjectExtension); - } - /** * Enable extensions. Then, exclude our JSON test module. As a result, the * JSON object will fail to deserialize. diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java index cd8cab32ca55..eca5617e479d 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java @@ -36,6 +36,7 @@ import org.apache.druid.guice.StartupInjectorBuilder; import org.apache.druid.initialization.CoreInjectorBuilder; import org.apache.druid.initialization.DruidModule; +import org.apache.druid.initialization.ServiceInjectorBuilder; import org.apache.druid.java.util.common.RE; import org.apache.druid.java.util.common.io.Closer; import org.apache.druid.math.expr.ExprMacroTable; @@ -904,15 +905,8 @@ private SqlTestFramework(Builder builder) overrideModules.add(testSetupModule()); builder.componentSupplier.configureGuice(injectorBuilder, overrideModules); - // First override with the testSetupModule. It would be better if the existing tests were updated to not require - // this step of override as this is actually an indication that the objects aren't managing their bindings - // properly. This can cause obfuscation when trying to figure out where a binding/object is coming from, slowing - // developer productivity. For expediency, we add this extra layer of overrides just to make things work while - // work can then be done to fix the problematic configurations. It would be awesome if this override was no - // longer neede some day. - injectorBuilder.overrideCurrentGuiceModules(List.of(testSetupModule())); - - injectorBuilder.overrideCurrentGuiceModules(overrideModules); + ServiceInjectorBuilder serviceInjector = new ServiceInjectorBuilder(injectorBuilder); + serviceInjector.addAll(overrideModules); this.injector = injectorBuilder.build(); this.engine = builder.componentSupplier.createEngine(queryLifecycleFactory(), queryJsonMapper(), injector); From 85664ec988e758ecdd7a30d68c7e573908d72c3f Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Tue, 17 Dec 2024 15:43:08 +0530 Subject: [PATCH 4/9] Fix the tests --- .../main/java/org/apache/druid/guice/DruidInjectorBuilder.java | 1 - .../java/org/apache/druid/guice/DruidInjectorBuilderTest.java | 2 -- .../org/apache/druid/sql/calcite/util/SqlTestFramework.java | 2 +- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java b/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java index ddf8b9096f50..964acc31641e 100644 --- a/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java +++ b/server/src/main/java/org/apache/druid/guice/DruidInjectorBuilder.java @@ -219,7 +219,6 @@ public List modules() return modules; } - public Injector build() { return Guice.createInjector(modules); diff --git a/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java b/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java index 050693268541..d55ebe8f16b1 100644 --- a/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java +++ b/server/src/test/java/org/apache/druid/guice/DruidInjectorBuilderTest.java @@ -23,7 +23,6 @@ import com.fasterxml.jackson.annotation.JsonTypeName; import com.fasterxml.jackson.databind.Module; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.jsontype.NamedType; import com.fasterxml.jackson.databind.module.SimpleModule; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -36,7 +35,6 @@ import org.apache.druid.initialization.CoreInjectorBuilder; import org.apache.druid.initialization.DruidModule; import org.apache.druid.java.util.common.ISE; -import org.junit.Assert; import org.junit.Test; import javax.inject.Inject; diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java index eca5617e479d..a812f9ffa87d 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java @@ -908,7 +908,7 @@ private SqlTestFramework(Builder builder) ServiceInjectorBuilder serviceInjector = new ServiceInjectorBuilder(injectorBuilder); serviceInjector.addAll(overrideModules); - this.injector = injectorBuilder.build(); + this.injector = serviceInjector.build(); this.engine = builder.componentSupplier.createEngine(queryLifecycleFactory(), queryJsonMapper(), injector); componentSupplier.configureJsonMapper(queryJsonMapper()); componentSupplier.finalizeTestFramework(this); From 3d5c05f4856de61219048ba19bd3f51f83913325 Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Tue, 17 Dec 2024 15:44:01 +0530 Subject: [PATCH 5/9] Try using maven:3-openjdk-17-slim --- distribution/docker/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/distribution/docker/Dockerfile b/distribution/docker/Dockerfile index 0ac517a29ba1..92af1915ee7d 100644 --- a/distribution/docker/Dockerfile +++ b/distribution/docker/Dockerfile @@ -23,7 +23,7 @@ ARG JDK_VERSION=17 # This is because it's not able to build the distribution on arm64 due to dependency problem of web-console. See: https://github.com/apache/druid/issues/13012 # Since only java jars are shipped in the final image, it's OK to build the distribution on x64. # Once the web-console dependency problem is resolved, we can remove the --platform directive. -FROM --platform=linux/amd64 maven:3.8.4-openjdk-17-slim as builder +FROM --platform=linux/amd64 maven:3-openjdk-17-slim as builder # Rebuild from source in this stage # This can be unset if the tarball was already built outside of Docker From 33bae50224f3767613a786b4389f811dbd9e7ac2 Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Tue, 17 Dec 2024 17:25:44 +0530 Subject: [PATCH 6/9] Try enabling debugging for mvn command --- distribution/docker/Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/distribution/docker/Dockerfile b/distribution/docker/Dockerfile index 92af1915ee7d..e98aef75ad2b 100644 --- a/distribution/docker/Dockerfile +++ b/distribution/docker/Dockerfile @@ -23,7 +23,7 @@ ARG JDK_VERSION=17 # This is because it's not able to build the distribution on arm64 due to dependency problem of web-console. See: https://github.com/apache/druid/issues/13012 # Since only java jars are shipped in the final image, it's OK to build the distribution on x64. # Once the web-console dependency problem is resolved, we can remove the --platform directive. -FROM --platform=linux/amd64 maven:3-openjdk-17-slim as builder +FROM --platform=linux/amd64 maven:3.8.4-openjdk-17-slim as builder # Rebuild from source in this stage # This can be unset if the tarball was already built outside of Docker @@ -40,7 +40,7 @@ RUN --mount=type=cache,target=/root/.m2 if [ "$BUILD_FROM_SOURCE" = "true" ]; th install \ -Pdist,bundle-contrib-exts \ -Pskip-static-checks,skip-tests \ - -Dmaven.javadoc.skip=true -T1C \ + -Dmaven.javadoc.skip=true -T1C -X \ ; fi RUN --mount=type=cache,target=/root/.m2 VERSION=$(mvn -B -q org.apache.maven.plugins:maven-help-plugin:3.2.0:evaluate \ From 7f89da73194af74c229220999cf6519cbeba9d2a Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Tue, 17 Dec 2024 19:52:19 +0530 Subject: [PATCH 7/9] Use maven:3.9 image --- distribution/docker/Dockerfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/distribution/docker/Dockerfile b/distribution/docker/Dockerfile index e98aef75ad2b..6b16ec2b94f3 100644 --- a/distribution/docker/Dockerfile +++ b/distribution/docker/Dockerfile @@ -23,7 +23,7 @@ ARG JDK_VERSION=17 # This is because it's not able to build the distribution on arm64 due to dependency problem of web-console. See: https://github.com/apache/druid/issues/13012 # Since only java jars are shipped in the final image, it's OK to build the distribution on x64. # Once the web-console dependency problem is resolved, we can remove the --platform directive. -FROM --platform=linux/amd64 maven:3.8.4-openjdk-17-slim as builder +FROM --platform=linux/amd64 maven:3.9 as builder # Rebuild from source in this stage # This can be unset if the tarball was already built outside of Docker @@ -40,7 +40,7 @@ RUN --mount=type=cache,target=/root/.m2 if [ "$BUILD_FROM_SOURCE" = "true" ]; th install \ -Pdist,bundle-contrib-exts \ -Pskip-static-checks,skip-tests \ - -Dmaven.javadoc.skip=true -T1C -X \ + -Dmaven.javadoc.skip=true -T1C \ ; fi RUN --mount=type=cache,target=/root/.m2 VERSION=$(mvn -B -q org.apache.maven.plugins:maven-help-plugin:3.2.0:evaluate \ From cd26dda5a5f3ad7eb830bf766d3dfe3f62cc285c Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Tue, 17 Dec 2024 19:59:47 +0530 Subject: [PATCH 8/9] Address review comment: Fix formatting --- .../org/apache/druid/query/DruidProcessingConfigTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java b/processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java index 9f4aea66f368..2be92dcd31ea 100644 --- a/processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java +++ b/processing/src/test/java/org/apache/druid/query/DruidProcessingConfigTest.java @@ -135,10 +135,12 @@ public void testInvalidSizeBytes() props ); - ExceptionMatcher.of(ProvisionException.class) + ExceptionMatcher + .of(ProvisionException.class) .expectMessageContains( "Cannot construct instance of `HumanReadableBytes`, problem: Invalid format of number: -1. Negative value is not allowed." - ).assertThrowsAndMatches(() -> injector.getInstance(DruidProcessingConfig.class)); + ) + .assertThrowsAndMatches(() -> injector.getInstance(DruidProcessingConfig.class)); } @Test From dae8703dead4a55b01e0e1b0d67a3b5ca13b5146 Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Tue, 17 Dec 2024 23:09:00 +0530 Subject: [PATCH 9/9] Address review comment: Add brief javadoc for ExceptionMatcher --- .../test/java/org/apache/druid/error/ExceptionMatcher.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/processing/src/test/java/org/apache/druid/error/ExceptionMatcher.java b/processing/src/test/java/org/apache/druid/error/ExceptionMatcher.java index de9b2a9728cc..31195093254f 100644 --- a/processing/src/test/java/org/apache/druid/error/ExceptionMatcher.java +++ b/processing/src/test/java/org/apache/druid/error/ExceptionMatcher.java @@ -29,6 +29,10 @@ import java.util.ArrayList; +/** + * A matcher for validating exceptions in unit tests, providing a fluent API for constructing matchers to allow + * matching of {@link Throwable} objects, such as verifying exception type, message content, and cause. + */ public class ExceptionMatcher extends DiagnosingMatcher {