Skip to content

#16717 defer provider instantiation in Kubernetes Module#16726

Merged
georgew5656 merged 3 commits intoapache:masterfrom
trompa:kubernetes-defer-provider-selector
Jul 16, 2024
Merged

#16717 defer provider instantiation in Kubernetes Module#16726
georgew5656 merged 3 commits intoapache:masterfrom
trompa:kubernetes-defer-provider-selector

Conversation

@trompa
Copy link
Copy Markdown
Contributor

@trompa trompa commented Jul 11, 2024

Fixes #16717

Description

Hadoop ingestion doesnt have access to K8s config. Provider not being pure lazy was making the HadoopIndexTask fail altho it doesnt really need K8s information on the Hadoop cluster.

This PR refactors the DruidLeaderSelectorProvider class so its creation can be deferred to Module instantiation, thus not happening on HadoopIndexTask

It creates 2 subclasses CoordinatorDruidLeaderSelectorProvider and IndexingServiceDruidLeaderSelectorProvider which can be properly referenced.

Was not sure about either overriding the get method on children or keep the parents, so i decided to take the option with less code changes.
Let me know if the other option id preferred.

Release note


Key changed/added classes in this PR
  • Fixed: Hadoop ingestion now works on k8s deployments

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • [ X] 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.
  • [ X] been tested in a test Druid cluster.

@trompa
Copy link
Copy Markdown
Contributor Author

trompa commented Jul 13, 2024

Tests failing:

  • style - Sorry! i thought my IntelliJ had applyed the style but it didnt ...
  • jacoco - Checked Curator Leader Selector tests, but cannot find any example test on how to test this provider. But, even if the class has been extracted it is just initialization code, so added it to the ignores.

Let me now if that's ok or how can i test the module provider

@trompa trompa requested a review from georgew5656 July 15, 2024 11:22
@trompa
Copy link
Copy Markdown
Contributor Author

trompa commented Jul 16, 2024

208.8 Caused by: org.eclipse.aether.transfer.ArtifactNotFoundException: Could not find artifact org.apache.druid.extensions.contrib:aliyun-oss-extensions:jar:31.0.0-SNAPSHOT in (https://repo1.maven.org/maven2/)

On docker build.

Dont know why this failed, but completely unrelated to my changes id say

@georgew5656
Copy link
Copy Markdown
Contributor

i think the gha runner was probably killed during the build, it succeeded when i retried. thanks for the change.

@georgew5656 georgew5656 merged commit ebf2168 into apache:master Jul 16, 2024
edgar2020 pushed a commit to edgar2020/druid that referenced this pull request Jul 19, 2024
…e#16726)

* apache#16717 defer provider instatiation

* add license header

* fix style, ignore new class in jacoco as it is still initialization code

---------

Co-authored-by: Alberto Lago Alvarado <albl@sitecore.net>
edgar2020 pushed a commit to edgar2020/druid that referenced this pull request Jul 19, 2024
…e#16726)

* apache#16717 defer provider instatiation

* add license header

* fix style, ignore new class in jacoco as it is still initialization code

---------

Co-authored-by: Alberto Lago Alvarado <albl@sitecore.net>
sreemanamala pushed a commit to sreemanamala/druid that referenced this pull request Aug 6, 2024
…e#16726)

* apache#16717 defer provider instatiation

* add license header

* fix style, ignore new class in jacoco as it is still initialization code

---------

Co-authored-by: Alberto Lago Alvarado <albl@sitecore.net>
@kfaraz kfaraz added this to the 31.0.0 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

K8s MM-less Hadoop ingestion fails on K8sDiscoveryModule

4 participants