kvserver,allocator: decouple allocator from storepool#91941
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Dec 22, 2022
Merged
kvserver,allocator: decouple allocator from storepool#91941craig[bot] merged 2 commits intocockroachdb:masterfrom
craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
0ac8798 to
874eae8
Compare
a1a2ace to
c35fe91
Compare
AlexTalks
added a commit
to AlexTalks/cockroach
that referenced
this pull request
Nov 23, 2022
Adds support for using the allocator to compute the action, if any, needed to "repair" the range and, if the action requires a replica addition (including replacements), the target of that addition. This can be checked by using the new `CheckRangeAction(..)` function as part of a store's replicate queue, and can use the default store pool or an override store pool, so that potential states can be evaluated prior to transition to those states. As such, this feature adds support for allocator action and target validation prior to decommission, in order to support decommission pre-checks. While this is similar to the replicate queue's `PlanOneChange(..)`, this new check supports evaluation based on a range descriptor, rather than an individual replica. Depends on cockroachdb#91941 Part of cockroachdb#91570. Release note: None
c35fe91 to
1468022
Compare
AlexTalks
added a commit
to AlexTalks/cockroach
that referenced
this pull request
Nov 23, 2022
Adds support for using the allocator to compute the action, if any, needed to "repair" the range and, if the action requires a replica addition (including replacements), the target of that addition. This can be checked by using the new `CheckRangeAction(..)` function as part of a store's replicate queue, and can use the default store pool or an override store pool, so that potential states can be evaluated prior to transition to those states. As such, this feature adds support for allocator action and target validation prior to decommission, in order to support decommission pre-checks. While this is similar to the replicate queue's `PlanOneChange(..)`, this new check supports evaluation based on a range descriptor, rather than an individual replica. Depends on cockroachdb#91941 Part of cockroachdb#91570. Release note: None
1468022 to
758b541
Compare
AlexTalks
added a commit
to AlexTalks/cockroach
that referenced
this pull request
Dec 15, 2022
Adds support for using the allocator to compute the action, if any, needed to "repair" the range and, if the action requires a replica addition (including replacements), the target of that addition. This can be checked by using the new `CheckRangeAction(..)` function as part of a store's replicate queue, and can use the default store pool or an override store pool, so that potential states can be evaluated prior to transition to those states. As such, this feature adds support for allocator action and target validation prior to decommission, in order to support decommission pre-checks. While this is similar to the replicate queue's `PlanOneChange(..)`, this new check supports evaluation based on a range descriptor, rather than an individual replica. Depends on cockroachdb#91941 Part of cockroachdb#91570. Release note: None
758b541 to
21b28d3
Compare
AlexTalks
added a commit
to AlexTalks/cockroach
that referenced
this pull request
Dec 19, 2022
Adds support for using the allocator to compute the action, if any, needed to "repair" the range and, if the action requires a replica addition (including replacements), the target of that addition. This can be checked by using the new `CheckRangeAction(..)` function as part of a store's replicate queue, and can use the default store pool or an override store pool, so that potential states can be evaluated prior to transition to those states. As such, this feature adds support for allocator action and target validation prior to decommission, in order to support decommission pre-checks. While this is similar to the replicate queue's `PlanOneChange(..)`, this new check supports evaluation based on a range descriptor, rather than an individual replica. Depends on cockroachdb#91941 Part of cockroachdb#91570. Release note: None
kvoli
reviewed
Dec 19, 2022
Contributor
kvoli
left a comment
There was a problem hiding this comment.
I'm not a fan of the decorated methods ...WithStorePool(...). It adds to the complexity of the allocator API by exposing additional methods.
Would it be possible to instead pass it as an argument for every top level function instead?
This also relates to the approach taken on subsequent PRs - is it possible to use planOneChange in place of any forked methods.
Reviewed 2 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks and @andrewbaptist)
-- commits line 4 at r1:
nit: drop the "to be"
AlexTalks
commented
Dec 19, 2022
Contributor
Author
AlexTalks
left a comment
There was a problem hiding this comment.
This is doable - let me know if you think it makes more sense to always pass in the StorePool on every allocator method (which may complicate things with callers), or if it would make sense to simply add an override parameter, like:
func (a *Allocator) ComputeAction(..., overrideStorePool storepool.AllocatorStorePool) {
storePool := a.StorePool
if overrideStorePool != nil {
storePool = overrideStorePool
}
...
}
Reviewable status:
26a63b1 to
e9507ff
Compare
e9507ff to
79e7b0a
Compare
kvoli
approved these changes
Dec 21, 2022
Contributor
kvoli
left a comment
There was a problem hiding this comment.
Reviewed 1 of 9 files at r3, 23 of 23 files at r4, all commit messages.
Reviewable status:
pkg/kv/kvserver/replicate_queue.go line 614 at r4 (raw file):
) (shouldQueue bool, priority float64) { desc, conf := repl.DescAndSpanConfig() action, priority := rq.allocator.ComputeAction(ctx, rq.store.cfg.StorePool, conf, desc)
I'm thinking it would be better if the replicate queue were instantiated with a ref to the StorePool similar to the store rebalancer.
pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 512 at r4 (raw file):
// MakeAllocator creates a new allocator. // The deterministic flag indicates that this allocator is intended to be used
What if it is not? This seems pretty fragile. There is a deterministic test which should catch this however, so it should be okay if any unlucky soul runs into it.
This change adds methods to evaluate allocator actions and targets utilizing a passed-in `StorePool` object, allowing for the allocator to consider potential scenarios rather than those simply based on the current liveness. Depends on cockroachdb#91461, cockroachdb#91965. Part of cockroachdb#91570. Release note: None
This fully decouples the StorePool from the allocator by moving ownership of the configured view of peer stores to the store configuration (the original source), the store rebalancer, and various structures in the allocator simulator. Part of cockroachdb#91570. Release note: None
79e7b0a to
5236ee1
Compare
Contributor
Author
|
bors r+ |
Contributor
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Jan 7, 2023
94114: kvserver: refactor replicate queue to enable allocator code reuse r=AlexTalks a=AlexTalks
This change refactors parts of the replicate queue's `PlanOneChange(..)`
and `addOrRemove{Non}Voters(..)` functions to reusable helper functions
that simplify usage of the allocator and deduplicate repeated code
paths. The change also adds convenience methods to the `AllocatorAction`
enum, to move certain determinations (such as if a computed allocator
action is a remove or a replace) closer to the allocator type it is
based on. These changes further enable the ability to use the allocator
outside of the replicate queue, and enable the ability for some of this
logic to move into the allocator itself in the future.
Depends on #91941.
Epic: none
Release note: None
Co-authored-by: Alex Sarkesian <sarkesian@cockroachlabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change refactors the allocator to accept the store pool as an
argument, moving it closer to being stateless and purely functional.
This is done by moving to using the storepool through its primary
owner (the store configuration), or through references held by
users of the allocator. The refactoring enables the the allocator to
consider potential scenarios rather than those simply based on the
current liveness.
Part of #91570.
Release Note: None