Skip to content

Make http options the default configurations#13092

Merged
kfaraz merged 6 commits intoapache:masterfrom
AmatyaAvadhanula:feature-http_configs_default
Oct 5, 2022
Merged

Make http options the default configurations#13092
kfaraz merged 6 commits intoapache:masterfrom
AmatyaAvadhanula:feature-http_configs_default

Conversation

@AmatyaAvadhanula
Copy link
Copy Markdown
Contributor

Makes http configurations the default configurations

Description

Druid uses Zookeeper dependent options as the default.

Http based options have been tried and tested and are now the recommended options

TaskRunner : local -> httpRemote

LoadQueue : curator -> http

ServerInventoryView : batch -> http



This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • 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.

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

Concept looks good to me!

"druid.indexer.runner.type",
Key.get(TaskRunnerFactory.class),
Key.get(ForkingTaskRunnerFactory.class)
Key.get(HttpRemoteTaskRunnerFactory.class)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With this change, I think there's no need for druid.indexer.runner.type=httpRemote in all the bundled configs anymore. Could you try removing that line and see if everything still works OK?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@cryptoe
Copy link
Copy Markdown
Contributor

cryptoe commented Sep 16, 2022

How will the upgrade look when people go from zk-based task management to HTTP-based task management considering most of the people running their clusters would be using default values?

IMHO we should tag release notes on this PR as well.

@kfaraz
Copy link
Copy Markdown
Contributor

kfaraz commented Oct 4, 2022

Upgrade should not be a concern for the loadqueue peon type and the server inventory view mode being changed to "http" as these take effect when the coordinator is restarted. The respective endpoints are already exposed by the historical, so the change should be seamless.

Task management should be fine too if the upgrade order is maintained i.e. MM/indexer before overlord. An upgraded middle manager would be listening on HTTP for new tasks but the overlord would start sending HTTP task assignments only when it is upgraded too.

cc: @cryptoe

@kfaraz
Copy link
Copy Markdown
Contributor

kfaraz commented Oct 4, 2022

@AmatyaAvadhanula , there seems to be some coverage failure. I think it can be remedied if you add a line to verify the loadqueue peon type in DruidCoordinatorConfigTest.

Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

LGTM

@AmatyaAvadhanula
Copy link
Copy Markdown
Contributor Author

@kfaraz thank you for the review! I've added the test

@kfaraz kfaraz merged commit 41e51b2 into apache:master Oct 5, 2022
@kfaraz kfaraz added this to the 25.0 milestone Nov 22, 2022
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.

4 participants