Skip to content

Select broker based on query context parameter brokerService#11495

Merged
abhishekagarwal87 merged 7 commits intoapache:masterfrom
kfaraz:select_broker_by_query_context
Jul 27, 2021
Merged

Select broker based on query context parameter brokerService#11495
abhishekagarwal87 merged 7 commits intoapache:masterfrom
kfaraz:select_broker_by_query_context

Conversation

@kfaraz
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz commented Jul 26, 2021 •
edited
Loading

Description

This change allows the selection of a specific broker service (or broker tier) by the Router.

The newly added ManualTieredBrokerSelectorStrategy works as follows:

  • Check for the parameter brokerService in the query context. If this is a valid broker service, use it.
  • Check if the field defaultManualBrokerService has been set in the strategy. If this is a valid broker service, use it.
  • Move on to the next strategy

Changes in this PR

  • Add class ManualTieredBrokerSelectorStrategy
  • Add parameter brokerService in QueryContexts
  • Update docs

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.

@kfaraz kfaraz changed the title Add Query Context flag brokerServiceName and corresponding broker selector strategy Add query context parameter brokerServiceName and corresponding broker selector strategy Jul 26, 2021
@kfaraz kfaraz marked this pull request as ready for review July 27, 2021 04:32
@kfaraz kfaraz changed the title Add query context parameter brokerServiceName and corresponding broker selector strategy Add query context parameter brokerService and corresponding broker selector strategy Jul 27, 2021
@kfaraz kfaraz changed the title Add query context parameter brokerService and corresponding broker selector strategy Select broker based on query context parameter brokerService Jul 27, 2021
abhishekagarwal87
abhishekagarwal87 approved these changes Jul 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@abhishekagarwal87 abhishekagarwal87 left a comment

Choose a reason for hiding this comment

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

@kfaraz - LGTM except some minor comments. can we rename queryContext strategy to something less implementation specific e.g. manual?

Comment thread ...r/src/main/java/org/apache/druid/server/router/QueryContextTieredBrokerSelectorStrategy.java Outdated
if (isValidBrokerService(contextBrokerService, tierConfig)) {
// If the broker service in the query context is valid, use that
return Optional.of(contextBrokerService);
} else if (isValidBrokerService(fallbackBrokerService, tierConfig)) {
Copy link
Copy Markdown
Contributor

@abhishekagarwal87 abhishekagarwal87 Jul 27, 2021

Choose a reason for hiding this comment

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

could we perform this check in the constructor itself? your class would have Optional broker that would either be Optional.absent() or Optional.of(fallbackBrokerService). you can return the field as it is.

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We don't have the TieredBrokerConfig in the constructor, because the object is being constructed from deserialization of config and not by Guice.

Comment thread docs/design/router.md Outdated

#### queryContext

This strategy reads the parameter `brokerService` from the query context and routes the query accordingly. If no valid `brokerService` is specified in the query context, the field `fallbackBrokerService` is used if set to a valid non-null value.
Copy link
Copy Markdown
Contributor

@abhishekagarwal87 abhishekagarwal87 Jul 27, 2021

Choose a reason for hiding this comment

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

can we call fallbackBrokerService defaultBrokerService instead? Latter looks more common in druid codebase.

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Jul 27, 2021

Choose a reason for hiding this comment

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

Renamed to defaultManualBrokerService

abhishekagarwal87
abhishekagarwal87 reviewed Jul 27, 2021
View reviewed changes
Comment thread docs/design/router.md Outdated
@kfaraz @abhishekagarwal87
Update docs/design/router.md
5f2b0ae 
Co-authored-by: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com>
maytasm
maytasm reviewed Jul 27, 2021
View reviewed changes
Comment thread server/src/main/java/org/apache/druid/server/router/ManualTieredBrokerSelectorStrategy.java Outdated
// If the fallbackBrokerService is valid, use that
return Optional.of(defaultManualBrokerService);
} else {
log.debug(
Copy link
Copy Markdown
Contributor

@maytasm maytasm Jul 27, 2021

Choose a reason for hiding this comment

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

Should this be error level or at least warning to indicate that some config (either queryContext and/or defaultManualBrokerService) are set incorrect?

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Jul 27, 2021

Choose a reason for hiding this comment

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

Fixed.

maytasm
maytasm reviewed Jul 27, 2021
View reviewed changes
Comment thread docs/design/router.md
```json
{
"type": "manual",
"defaultManualBrokerService": "druid:broker-hot"
Copy link
Copy Markdown
Contributor

@maytasm maytasm Jul 27, 2021

Choose a reason for hiding this comment

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

is defaultManualBrokerService required?

Copy link
Copy Markdown
Contributor Author

@kfaraz kfaraz Jul 27, 2021

Choose a reason for hiding this comment

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

No, it is an optional field.

maytasm
maytasm approved these changes Jul 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@maytasm maytasm left a comment

Choose a reason for hiding this comment

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

Minor comments but overall LGTM

@abhishekagarwal87 abhishekagarwal87 merged commit 8a4e27f into apache:master Jul 27, 2021
@abhishekagarwal87
Copy link
Copy Markdown
Contributor

abhishekagarwal87 commented Jul 27, 2021

Merging since CI failure (kafka integration test) is unrelated

@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
@clintropolis clintropolis mentioned this pull request Sep 3, 2021
Closed
@kfaraz kfaraz deleted the select_broker_by_query_context branch August 1, 2023 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abhishekagarwal87 abhishekagarwal87 abhishekagarwal87 approved these changes

@maytasm maytasm maytasm approved these changes

Assignees

No one assigned

Labels

Area - Querying Design Review Release Notes

Projects

None yet

Milestone

0.22.0

Development

Successfully merging this pull request may close these issues.

4 participants

@kfaraz @abhishekagarwal87 @maytasm @clintropolis