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

Coordinator segment balancer max load queue fix #5888

Merged

Conversation

clintropolis
Copy link
Member

DruidCoordinatorBalancer will now respect config setting maxSegmentsInNodeLoadingQueue, filtering servers which cross the threshold from potential move targets.

Related to #5882

@clintropolis
Copy link
Member Author

clintropolis commented Jun 19, 2018

This fix as it is currently, will remove servers with a 'full' load queue from consideration as both a destination to move segments to, as well as a source to move segments from. If it is more preferable to still allow moving segments off of these servers, the server list could be duplicated and pick segments from the full set, but on consider moving to the filtered set.

@jon-wei
Copy link
Contributor

jon-wei commented Jun 19, 2018

Cool, I think it'd be good to still allow moves from servers that have a full load queue

@@ -103,6 +103,7 @@ private void balanceTier(
return;
}

final List<ServerHolder> entireServerHolderList = Lists.newArrayList(servers);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename these to "sourceServerHolderList" and "destinationServerHolderList"

Copy link
Member Author

Choose a reason for hiding this comment

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

didn't use those exactly, but gave those and some other variables some better names

@clintropolis
Copy link
Member Author

Updated to let segments still be moved from servers with a full load queue

Copy link
Contributor

@jon-wei jon-wei left a comment

Choose a reason for hiding this comment

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

👍 after CI

@gianm gianm merged commit 1a7adab into apache:master Jun 21, 2018
gianm pushed a commit to implydata/druid-public that referenced this pull request Jun 21, 2018
* Coordinator segment balancer will now respect "maxSegmentsInNodeLoadingQueue" config

* allow moves from full load queues

* better variable names
@clintropolis clintropolis deleted the coordinator-balancer-honor-max-loading-queue branch July 4, 2018 00:08
@jihoonson jihoonson added this to the 0.12.2 milestone Jul 5, 2018
@jihoonson jihoonson added the Bug label Jul 5, 2018
jihoonson pushed a commit to jihoonson/druid that referenced this pull request Jul 7, 2018
* Coordinator segment balancer will now respect "maxSegmentsInNodeLoadingQueue" config

* allow moves from full load queues

* better variable names
gianm pushed a commit that referenced this pull request Jul 9, 2018
* Coordinator segment balancer will now respect "maxSegmentsInNodeLoadingQueue" config

* allow moves from full load queues

* better variable names
leventov pushed a commit to metamx/druid that referenced this pull request Jul 20, 2018
…#5970)

* Coordinator segment balancer will now respect "maxSegmentsInNodeLoadingQueue" config

* allow moves from full load queues

* better variable names
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.

None yet

4 participants