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

Remove deprecated coordinator dynamic configs #14923

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

kfaraz
Copy link
Contributor

@kfaraz kfaraz commented Aug 29, 2023

Changes

Remove config decommissioningMaxPercentOfMaxSegmentsToMove

  • It is a complicated config 😅 ,
  • It is always desirable to prioritize move from decommissioning servers so that they can be terminated quickly, so this should always be 100%
  • It is already handled by smartSegmentLoading (enabled by default)

Remove config maxNonPrimaryReplicantsToLoad

This was added in #11135 to address two requirements:

  • Prevent coordinator runs from getting stuck assigning too many segments to historicals
  • Prevent load of replicas from competing with load of unavailable segments

Both of these requirements are now already met thanks to:

  • Round-robin segment assignment: This ensures that asssignments in a single coordinator run happen very fast
  • Prioritization in the new coordinator: Load of unavailable segments is always prioritized over load of under-replicated segment
  • Modifications to replicationThrottleLimit: The replicationThrottleLimit now ensures that for any tier, the sum of the number of replicas in load queue at start of a coordinator run and the number of replicas assigned during the run is always within the limit. So, it can perform the same function as maxNonPrimaryReplicantsToLoad except replicationThrottleLimit is per tier.
  • smartSegmentLoading (enabled by default) takes care of all of the above without requiring the user to change any config

Pending

  • Cluster testing

Release note

Remove deprecated coordinator dynamic configs:

  • decommissioningMaxPercentOfMaxSegmentsToMove
  • maxNonPrimaryReplicantsToLoad
    These configs are not needed anymore as smartSegmentLoading automatically ensures the optimal behaviour in the underlying scenarios.

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.

Copy link
Contributor

@AmatyaAvadhanula AmatyaAvadhanula left a comment

Choose a reason for hiding this comment

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

LGTM @kfaraz!

@kfaraz kfaraz merged commit ec630e3 into apache:master Sep 4, 2023
74 checks passed
@kfaraz kfaraz deleted the remove_dynamic_configs branch September 4, 2023 06:24
@kfaraz
Copy link
Contributor Author

kfaraz commented Sep 4, 2023

Thanks for the review, @AmatyaAvadhanula !

jakubmatyszewski pushed a commit to jakubmatyszewski/druid that referenced this pull request Sep 8, 2023
Changes:

[A] Remove config `decommissioningMaxPercentOfMaxSegmentsToMove`
- It is a complicated config 😅 , 
- It is always desirable to prioritize move from decommissioning servers so that
they can be terminated quickly, so this should always be 100%
- It is already handled by `smartSegmentLoading` (enabled by default)

[B] Remove config `maxNonPrimaryReplicantsToLoad`
This was added in apache#11135 to address two requirements:
- Prevent coordinator runs from getting stuck assigning too many segments to historicals
- Prevent load of replicas from competing with load of unavailable segments

Both of these requirements are now already met thanks to:
- Round-robin segment assignment
- Prioritization in the new coordinator
- Modifications to `replicationThrottleLimit`
- `smartSegmentLoading` (enabled by default)
@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