Tidy up construction of the Guice Injectors by paul-rogers · Pull Request #12816 · apache/druid

paul-rogers · 2022-07-24T02:19:25Z

See Issue #12815 for an overview of the goal of this PR. This PR provides the basic refactoring to allow a later PR to restructure the Calcite test setup which will in turn simply the proposed Druid planner test framework. This PR also lays the groundwork to simplify the complex, ad-hoc code which the "new ITs" use to set up tests.

Refactors Guice Injector Construction

See the above issue for an overview.

`ExtensionsLoader`

The Initialization class currently performa a kitchen-sink set of tasks, one of which is to load extensions. That code was pulled out into a separate class: ExtensionsLoader. The previous code used global maps to hold extension information, and cobbled together a way to pass a config object into static Initialization methods.

The new approach is that ExtensionsLoader is Guice-enabled: it is created by the startup injector and passed to the primary injector. Guice, then, manages the singleton for this class. Clients of the code now obtain an ExtensionsLoader from Guice.

InitializationTest cases related to extensions moved into a new ExtensionsLoaderTest class.

Hadoop Initialization

The Initialization class also holds a method, getHadoopDependencyFilesToLoad(), used only by Hadoop indexing tasks. This code moved to a new Initialization class in the indexing module. The test cases moved to a new test class.

`Initialization` Deprecation

The Initialization class is now virtually empty:

Hadoop-related code moved as described above.
Extension-related code moved as described above.
Module management moved into injector builders as described in Issue #12815.

The only functionality left in Initialization is makeInjectorWithModules(), now marked as deprecated, to build an injector. However, this method adds all the server dependencies, which is not what is wanted for tests. Over time, tests will be refactored to build an injector with just the modules needed, and without server baggage. At that time, we'll drop the makeInjectorWithModules() method and thus the Initialization class itself.

`GuiceInjectors` Deprecation

Similarly, the GuiceInjectors class builds the startup injector. As tests shift to use the builders, GuiceInjectors can disappear.

`CalciteTestsInjectorBuilder`

The "Calcite tests" has an elaborate set of ad-hoc code in CalciteTests to hand-wire a set of mock objects. The planner test PR struggled to refactor the code to allow more flexibility. A key motivation of this clean-up is to provide a framework that uses Guice to construct the Calcite test environment.

In this PR, we define a CalciteTestsInjectorBuilder class to build the injector that was previously built in a static initializer. This class is just a proof-of-concept at present: it is useful by itself. The real goal is to gradually move the ad-hoc code that creates planner-related object in Calcite tests to use this new builder (or a variation.)

This PR has:

been self-reviewed.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

lgtm-com · 2022-07-24T03:35:19Z

This pull request introduces 2 alerts when merging 7992d8a into 5772dfd - view on LGTM.com

new alerts:

2 for Spurious Javadoc @param tags

Builders for various module collections Revise the extensions loader Injector builders for server startup Move Hadoop init to indexer Clean up server node role filtering Calcite test injector builder

gianm

Looks like a nicer way of doing things.

gianm · 2022-07-30T19:50:23Z

indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java

-  static {
-    EXTENSIONS_CONFIG = INJECTOR.getInstance(ExtensionsConfig.class);
-  }
+  // Note: static variables mean that this task cannot run in a shared JVM,


Is this because ExtensionsLoader isn't concurrency-safe?

As far as I can tell, the prior code was OK to use in a shared JVM: it was an Injector (which I think is concurrency-safe?) and and ExtensionsConfig (immutable; definitely fine).

The issue here is more basic: static variables. If task A and task B both run in the Indexer, then they share a single copy of the static variable (unless the Indexer loads them in isolated class loaders.) By contrast, the ExtensionsLoader was converted from static variables to a Guice-injected singleton to avoid this issue. For ExtensionsLoader the only time two instances will appear in the same JVM is in tests, which used to have special code to undo the static variables between tests.

The HadoopTask approach seems more like a bug than a feature. A good enhancement would be to modify the HadoopTask to use Guice (and the ExtensionsLoader, perhaps modified for the Hadoop dependencies) to avoid the static variable issue.

Yeah, the HadoopTask is done in a weird way. But I was asking if anything bad happens due to sharing the statics between different tasks. The comment makes it sound like something will break, but I can't tell what it is. If nothing will break, it'd be better to get rid of (or reword) the comment.

Looking more closely, you may be right that this particular use is OK. There will be a single injector and set of extensions used by multiple tasks, which should be OK. Removed the comment.

gianm · 2022-07-31T17:12:01Z

...apache/druid/indexing/common/task/batch/parallel/MultiPhaseParallelIndexingRowStatsTest.java

  }

  @Test
+  @Ignore("assumes record rates, to be fixed in separate PR")


What's the issue here? I don't understand the comment.

Sorry, cryptic. This test failed repeatedly in this PR. Discussions with other engineers revealed that the test itself is flawed: it assumes the test duration. Someone else is fixing that. This temporary hack exists just so that this PR will build. I'll check if the required fix has been submitted yet, and if so, will remove this annotation.

Ah, I see. Is there a GitHub issue for the flaky test? It'd be good to link that here so the comment makes more sense.

Done. Sounds like #12852 will remove this test, so noted that here.

server/src/test/java/org/apache/druid/initialization/ServerInjectorBuilderTest.java

processing/src/main/java/org/apache/druid/guice/ExtensionsLoader.java

processing/src/main/java/org/apache/druid/guice/GuiceInjectors.java

server/src/main/java/org/apache/druid/initialization/Initialization.java

core/src/main/java/org/apache/druid/initialization/DruidModule.java

server/src/main/java/org/apache/druid/initialization/ServerInjectorBuilder.java

paul-rogers · 2022-08-02T01:25:40Z

@gianm, thanks much for the review! Address your comments. You pointed out a couple of tender spots in areas I avoided changing. Please take a look at the responses so we can figure out if we need to do anything more.

The suggestion for the default implementation of getJacksonModules() was good. So good, in fact, I went wild and removed all the now-redundant stub implementations. Let's see what IntelliJ inspections thinks of my handiwork before doing another review.

gianm · 2022-08-02T19:22:59Z

@gianm, thanks much for the review! Address your comments. You pointed out a couple of tender spots in areas I avoided changing. Please take a look at the responses so we can figure out if we need to do anything more.

I just wrote a few replies. When you have a chance to look at these I think this patch is good to go for me.

gianm

LGTM 🚀

paul-rogers marked this pull request as draft July 24, 2022 02:19

Refactor Guice initialization

e631781

Builders for various module collections Revise the extensions loader Injector builders for server startup Move Hadoop init to indexer Clean up server node role filtering Calcite test injector builder

paul-rogers force-pushed the 220723-guice branch from efd4bcc to e631781 Compare July 29, 2022 17:05

paul-rogers marked this pull request as ready for review July 30, 2022 19:32

gianm reviewed Jul 31, 2022

View reviewed changes

Revisions from review comments

8a8620c

Build fixes

9d951e1

Revisions from review comments

fc0893b

gianm approved these changes Aug 4, 2022

View reviewed changes

gianm merged commit a618458 into apache:master Aug 4, 2022

paul-rogers mentioned this pull request Aug 18, 2022

Tidy up Injector Construction for Tests, Clients and More #12815

Closed

abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022

Conversation

paul-rogers commented Jul 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactors Guice Injector Construction

ExtensionsLoader

Hadoop Initialization

Initialization Deprecation

GuiceInjectors Deprecation

CalciteTestsInjectorBuilder

Uh oh!

lgtm-com bot commented Jul 24, 2022

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

gianm Jul 30, 2022

Choose a reason for hiding this comment

Uh oh!

paul-rogers Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

gianm Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-rogers Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

gianm Jul 31, 2022

Choose a reason for hiding this comment

Uh oh!

paul-rogers Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

gianm Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

paul-rogers Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paul-rogers commented Aug 2, 2022

Uh oh!

gianm commented Aug 2, 2022

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paul-rogers commented Jul 24, 2022 •

edited

Loading

`ExtensionsLoader`

`Initialization` Deprecation

`GuiceInjectors` Deprecation

`CalciteTestsInjectorBuilder`

gianm Aug 2, 2022 •

edited

Loading