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

Add option for balancing a single table at a time when using host regex balancer #4523

Draft
wants to merge 2 commits into
base: 2.1
Choose a base branch
from

Conversation

cshannon
Copy link
Contributor

@cshannon cshannon commented May 3, 2024

This adds a new property balancer.host.regex.concurrent.tables which if true will process multiple tables at a time (this is the default). If set to false the balancer will stop processing after a table is found with migrations

This is a draft PR for now to start getting some feed back on the approach. I have implemented the 4 steps but not quite sure it's actually correct yet. I've been playing around with it for a little while tweaking things and trying to test but it hasn't been tested for real yet (other than the smiple test in BaseHostRegexTAbleLoadBalancerTest) yet.

I made the property a default of true for concurrent but we could negate that and make the property something like balancer.host.regex.single.table and default to false.

This closes #4521

alancer

This adds a new property balancer.host.regex.concurrent.tables which if
true will process multiple tables at a time (this is the default). If
set to false the balancer will stop processing after a table is found
with migrations
@cshannon cshannon self-assigned this May 3, 2024
@cshannon cshannon changed the title Add option for balancing a single table at a time when using host regex Add option for balancing a single table at a time when using host regex balancer May 3, 2024
@@ -466,6 +475,13 @@ public long balance(BalanceParameters params) {
LOG.trace("Sample up to 10 outstanding migrations: {}", limitTen(migrations));
}
return minBalanceTime;
} else if (!myConf.concurrentTables) {
LOG.warn("Not balancing tables due to {} existing migrations and {}} is set to false",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be at debug? It seems that it will be common and would be expected to sort itself out eventually. It would only be a concern is it was persistent? Could the message include something to show that it may be making progress? Current id being processed?

Is this a log message that quickly flood the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this should likely be debug or trace, I just copied it from above and didn't pay attention but that would definitely flood things pretty quickly and it is expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems another case where we really could benefit from being able to log 1 of Number of 1 every T seconds occurrences of the same message. It is not helpful to keep repeating that same message frequently, but having it appear occasionally helps troubleshoot cases where something in not making progress - might be out of scope for this, but could be worthwhile.

@ctubbsii ctubbsii added this to the 2.1.3 milestone Jul 12, 2024
@ctubbsii ctubbsii modified the milestones: 2.1.3, 2.1.4 Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants