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 balancer meter fixes #4470 #4471

Open
wants to merge 5 commits into
base: 2.1
Choose a base branch
from

Conversation

EdColeman
Copy link
Contributor

@EdColeman EdColeman commented Apr 18, 2024

Adds two gauges that can be used to monitor balancing

  • manager.balancer.balancing.total - snapshot of the number of current migrations being performed.
  • manager.balancer.need.balancing.total - snapshot of the total number of migrations needed to balancing.

This fixes #4470

@EdColeman EdColeman requested a review from ddanielr April 18, 2024 20:46
@EdColeman EdColeman self-assigned this Apr 18, 2024
@EdColeman EdColeman added this to In progress in 3.1.0 via automation Apr 18, 2024
@EdColeman EdColeman added this to In progress in 2.1.3 via automation Apr 18, 2024
@EdColeman
Copy link
Contributor Author

I confirmed that with uno instance configured to use a LoggingProvider, the meters appear in the manager metrics log:

METRICS: 2024-04-19T12:08:37,303, manager.balancer.balancing.total{host=localhost,instance.name=uno,port=9999,process.name=manager} value=0
METRICS: 2024-04-19T12:08:37,303, manager.balancer.need.balancing.total{host=localhost,instance.name=uno,port=9999,process.name=manager} value=0

@EdColeman EdColeman marked this pull request as ready for review April 24, 2024 17:01
Comment on lines +946 to +947
balancerMetrics.setMigratingCount(params.migrationsOut().size());
balancerMetrics.setNeedMigrationCount(migrations.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the migrations variable in the Manager contains all of the in-flight migrations. The params.migrationsOut contains the tablets that need to be migrated from this call to the balancer. I think based on what I'm seeing here, it's double counting. I wonder if we just want the size of the migrations list as that will encompass newly added, newly removed, and migrations waiting to happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to capture the newly added migrations, then it might make sense to capture the migration completions (which are in the TabletGroupWatcher) and cancellations (which happen in a couple places in the Manager).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2.1.3
In progress
3.1.0
In progress
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants