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

Removes a constructor from SimpleLoadBalancer #5363

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

Conversation

ddanielr
Copy link
Contributor

Removes the single TableId constructor from SimpleLoadBalancer and modifies the balancer logic to use the tablesToBalance method from the balancerParams.

This ensures that the SimpleLoadBalancer will only balance on a subset of tables vs balancing all tables.

Removes the single TableId constructor from SimpleLoadBalancer and
modifies the balancer logic to use the tablesToBalance method from the
balancerParams.
@ddanielr ddanielr added this to the 2.1.4 milestone Feb 27, 2025
@ddanielr ddanielr marked this pull request as draft February 27, 2025 22:22
@ddanielr
Copy link
Contributor Author

Seeing an issue with the TableLoadBalancerTest. Needs more investigation.

SimpleLoadBalancer did not have a multi-table balance test for a subset
of tables.
Added a test for balancing two of three total tables
@ddanielr ddanielr marked this pull request as ready for review March 3, 2025 19:14
@ddanielr
Copy link
Contributor Author

ddanielr commented Mar 4, 2025

Reviewed this code with @keith-turner and realized that removing this constructor actually breaks all user created table balancers that are supposed to be loaded by the TableLoadBalancer.

TableLoadBalancer requires user defined balancers to have a constructor that accepts a tableId, but we do not enforce that behavior with any interface in the SPI.
This is also not documented in the javadoc.

Leaving this open to make javadoc updates. But these changes will need to be done in main and not in 2.1

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.

2 participants