Skip to content

Lower the RelocateShard priority for removing redundant teams #1853

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.

Sign up for GitHub

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

Jump to bottom
Merged
etschannen merged 7 commits into from
Jul 26, 2019
Merged

Lower the RelocateShard priority for removing redundant teams #1853

etschannen merged 7 commits into from
Jul 26, 2019

Conversation

@xumengpanda
Copy link
Contributor

@xumengpanda xumengpanda commented Jul 17, 2019

This PR solves the Issue #1845.

We reduces the priority for removing redundant teams from 700 to 210, lower than merge or split shard priority.

@xumengpanda xumengpanda added this to the 6.2 milestone Jul 17, 2019
@xumengpanda
Copy link
Contributor Author

xumengpanda commented Jul 17, 2019

This turns out to be more complex than I thought.
The one line change is not enough.

I will wait for the server team remover PR to be merged and then get back to this PR.

@xumengpanda
Increase merge and split shard priority by 100
f243e77 
PRIORITY_TEAM_REDUNDANT should be in a different priority band from
PRIORITY_MERGE_SHARD and PRIORITY_SPLIT_SHARD, because
priority inversion happens within priorities in the same band.
@xumengpanda xumengpanda requested a review from jzhou77 July 19, 2019 22:25
jzhou77
jzhou77 reviewed Jul 19, 2019
View reviewed changes
@xumengpanda
Copy link
Contributor Author

xumengpanda commented Jul 20, 2019

Can you provide an example case where the priority inversion can happen?

The priority band is calculated by the priority / 100.
Each priority band has some amount of budget to move data in parallel.
If we put redundant team priority into the same band with merge/split shard priority, removing redundant team can take up the budget and stops the merge/split shard from running. That's how priority inversion happens.

@xumengpanda
Copy link
Contributor Author

xumengpanda commented Jul 20, 2019

Passed 100K random tests.
TODO: Run a local test to make sure the status is correct.

@xumengpanda
Copy link
Contributor Author

xumengpanda commented Jul 22, 2019

The local run without workload looks good.

@xumengpanda xumengpanda requested a review from jzhou77 July 22, 2019 17:56
jzhou77
jzhou77 reviewed Jul 22, 2019
View reviewed changes
fdbserver/Status.actor.cpp
} else if (highestPriority >= PRIORITY_TEAM_CONTAINS_UNDESIRED_SERVER) {
stateSectionObj["healthy"] = true;
stateSectionObj["name"] = "healthy_removing_server";
stateSectionObj["description"] = "Removing storage server";
Copy link
Contributor

@jzhou77 jzhou77 Jul 22, 2019

Choose a reason for hiding this comment

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

This message looks confusing/scary. Maybe something like "optimizing team servers"?

Copy link
Contributor Author

@xumengpanda xumengpanda Jul 22, 2019

Choose a reason for hiding this comment

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

This is for PRIORITY_TEAM_CONTAINS_UNDESIRED_SERVER which is not touched by this PR.

I'm inclined to keep what it is because undesired server (e.g., the server on the same address with another server) is supposed to be removed.

etschannen
etschannen reviewed Jul 22, 2019
View reviewed changes
@xumengpanda
Remove unnecessary condition in DDQueue
e582219 
Resolve the review comment.
@xumengpanda
Copy link
Contributor Author

xumengpanda commented Jul 23, 2019

@fdb-build test windows please

@xumengpanda
Copy link
Contributor Author

xumengpanda commented Jul 23, 2019

@fdb-build test this please

@xumengpanda
Copy link
Contributor Author

xumengpanda commented Jul 23, 2019

@fdb-build test this please

@xumengpanda
Copy link
Contributor Author

xumengpanda commented Jul 24, 2019

@etschannen This PR is ready to be merged.
The only change since our review session is removing the redundant condition in the if-statement.

@etschannen etschannen merged commit 2123fa1 into apple:master Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jzhou77 jzhou77 jzhou77 approved these changes

+1 more reviewer

@etschannen etschannen etschannen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@etschannen etschannen

Labels

data distribution

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

4 participants

@xumengpanda @dongxinEric @jzhou77 @etschannen