Skip to content

Datasource based exemption from deprioritization#17416

Closed
aruraghuwanshi wants to merge 7 commits intoapache:masterfrom
aruraghuwanshi:datasource-based-exemption-from-deprioritization
Closed

Datasource based exemption from deprioritization#17416
aruraghuwanshi wants to merge 7 commits intoapache:masterfrom
aruraghuwanshi:datasource-based-exemption-from-deprioritization

Conversation

@aruraghuwanshi
Copy link
Contributor

@aruraghuwanshi aruraghuwanshi commented Oct 25, 2024

Description

As of today tthere isn't a way to define high/low priority datasources. There are situations where we want to deprioritize queries based on the already present time based thresholds, but its possible that the druid cluster might host datasources that are higher in priority compared to others. For this reason, I've added another Optional json parameter exemptDatasources, which would host those high-priority datasources, which will skip the deprioritization logic entirely if the query being executed hosts any of the exempted datasources.

Updated the class ThresholdBasedQueryPrioritizationStrategy.

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.

@maytasm
Copy link
Contributor

maytasm commented Nov 5, 2024

Just my 2c,

  • If it's possible, having this as a dynamic config like Coordinator/OVerlord dynamic config would be useful. Often time we find that important query/datasouce was put into the low lane and we need to switch it back to high lane ASAP
  • An opposite of this would be useful to. Forcing datasource into low lane. Often time we find that bad/expensive query/datasouce starts coming in that, although not exceeding the threshold, is very very expensive CPU-wise (due to some query shape that was not captured in the current threshold settings)

@JsonProperty("durationThreshold") @Nullable String durationThresholdString,
@JsonProperty("segmentCountThreshold") @Nullable Integer segmentCountThreshold,
@JsonProperty("segmentRangeThreshold") @Nullable String segmentRangeThresholdString,
@JsonProperty("exemptDatasources") @Nullable Set<String> exemptDatasources,
Copy link
Contributor

@cryptoe cryptoe Nov 18, 2024

Choose a reason for hiding this comment

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

This might lead to more issues like one query hogging the cluster for the exempt data source and all your query lanning goes for a toss. Should we revert the logic, ie if an "lowPriorityDataSource" is hit, lower the priority always. That way the cluster can be saved against outages ? Its better to err on the side of caution no ?

Since there is no clear answer here :

  • What do other data bases do ?
  • What's your practical experience running this config in production.

@github-actions
Copy link

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 18, 2025
@github-actions
Copy link

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants