Skip to content

MSQ should load even if node roles are not set#13318

Merged
abhishekagarwal87 merged 1 commit intoapache:masterfrom
abhishekagarwal87:node_role_msq
Nov 7, 2022
Merged

MSQ should load even if node roles are not set#13318
abhishekagarwal87 merged 1 commit intoapache:masterfrom
abhishekagarwal87:node_role_msq

Conversation

@abhishekagarwal87
Copy link
Contributor

@abhishekagarwal87 abhishekagarwal87 commented Nov 7, 2022

Fixes #13276.

Release note

For tips about how to write a good release note, see Release notes.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • 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.
  • added integration tests.
  • been tested in a test Druid cluster.

@kfaraz kfaraz added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Nov 7, 2022
@Provides
@LazySingleton
public Bouncer makeBouncer(final DruidProcessingConfig processingConfig, @Self final Set<NodeRole> nodeRoles)
public Bouncer makeBouncer(final DruidProcessingConfig processingConfig, Injector injector)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current code, if the node roles are not set, does this Set<NodeRole> come out as empty or null?
Can't we just use a condition on that null or emptiness instead of using Injector?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue is that there is nothing binded to @self annotation so the injector usage seems correct to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a generic fix, we could also try adding ServerInjectorBuilder.registerNodeRoleModule to the HadoopTask injector with empty role list.
That will also require some change in registerNodeRoleModule to bind an empty set incase of an empty role list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the caller chain starts from a static initialization block in HadoopDruidIndexerConfig. It's hard to say where does this static block can be called from. (another reason to not like static initialization blocks)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the HadoopDruidIndexerConfig injector would also bind an empty set of node roles if we fix registerNodeRoleModule to do so. The current code flow of Initialization.makeInjectorWithModules itself never passes any node roles to the injector

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The current fix felt more generic to me. This is how it's done in the MetricsModule class as well. I am happy to do it that way too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the MetricsModule change might also be obsolete after the recent refactoring in injector creation. FWIU, the module is initialized in CoreInjectorBuilder and that builder should have the nodeRoleModule.
for generic solution, I was actually thinking of a way where we could always inject the node roles set from the bottom itself instead of fetching from the injector so that all extensions can just use the injection directly.

Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

@abhishekagarwal87 abhishekagarwal87 merged commit b1eaf7a into apache:master Nov 7, 2022
@kfaraz kfaraz added this to the 25.0 milestone Nov 22, 2022
599166320 pushed a commit to 599166320/apache-druid that referenced this pull request Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

upgrade to 24.0.0 from 0.23.0, Hadoop index task fail

4 participants