Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completely disable cachingCost balancer strategy #14798

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

kfaraz
Copy link
Contributor

@kfaraz kfaraz commented Aug 11, 2023

Description

cachingCost has been deprecated in #14484 and is not advised to be used in production clusters as it may cause usage skew across historicals which the coordinator is unable to rectify. This PR completely disables cachingCost strategy as it has now been rendered redundant due to recent performance improvements made to cost strategy.

Changes

  • Disable cachingCost strategy
  • Add DisabledCachingCostBalancerStrategyFactory for the time being so that we can give a proper error message before falling back to CostBalancerStrategy. This will be removed in subsequent releases.
  • Retain CachingCostBalancerStrategy and related classes for testing and benchmarking purposes.
  • Add javadocs to DiskCostBalancerStrategy.

Release note

cachingCost balancer strategy is now disabled and cannot be used in a Druid cluster. Using cachingCost falls back to cost balancer strategy.

@@ -26,7 +26,7 @@
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "strategy", defaultImpl = CostBalancerStrategyFactory.class)
@JsonSubTypes(value = {
@JsonSubTypes.Type(name = "cost", value = CostBalancerStrategyFactory.class),
@JsonSubTypes.Type(name = "cachingCost", value = CachingCostBalancerStrategyFactory.class),
Copy link
Contributor

Choose a reason for hiding this comment

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

CachingCostBalancerStrategyFactory should be removed from code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is being used in some coordinator simulations and I have kept it around for the time being for some benchmarking and correctness comparison with upcoming strategies. It will most likely be removed before the next release of Apache Druid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added javadoc to this effect.

@kfaraz kfaraz merged commit d9221e4 into apache:master Aug 16, 2023
74 checks passed
@kfaraz kfaraz deleted the disable_caching_cost_strat branch August 16, 2023 06:13
@LakshSingla LakshSingla added this to the 28.0 milestone Oct 12, 2023
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.

3 participants