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

Fixes balancing of metadata table #5195

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

Conversation

keith-turner
Copy link
Contributor

No description provided.

@keith-turner keith-turner modified the milestones: 4.0.0, 2.1.4 Dec 17, 2024
@keith-turner
Copy link
Contributor Author

Have not tested these changes. Made similar changes in 4.0 and manually tested them. We probably need an IT that test balancing the metadata table if there is not one. In 4.0 this issue was preventing the metadata table from balancing across multiple tablet servers.

@@ -1000,20 +1000,22 @@ private SortedMap<TServerInstance,TabletServerStatus> createTServerStatusView(
final Map<String,TableInfo> newTableMap =
new HashMap<>(dl == DataLevel.USER ? oldTableMap.size() : 1);
if (dl == DataLevel.ROOT) {
if (oldTableMap.containsKey(RootTable.NAME)) {
newTableMap.put(RootTable.NAME, oldTableMap.get(RootTable.NAME));
if (oldTableMap.containsKey(RootTable.ID.canonical())) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not make it a Map<TableId,TableInfo> instead of Map<String,TableInfo>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not make it a Map<TableId,TableInfo> instead of Map<String,TableInfo>?

The code is currently manipulating a Thrift object which can not use the TableId type. A larger refactor would be needed to make that change, could open in issue to look into that.

@keith-turner
Copy link
Contributor Author

Working on adding an IT for metadata table balancing.

@keith-turner keith-turner marked this pull request as draft December 18, 2024 23:21
@keith-turner
Copy link
Contributor Author

Wrote a test and found some additional issues and made fixes to get the test to pass. Converted this PR to draft because I want to go review the changes I made to get the test to pass.

@keith-turner keith-turner marked this pull request as ready for review December 21, 2024 00:22
@keith-turner
Copy link
Contributor Author

keith-turner commented Dec 21, 2024

After writing the new IT I saw the following problems, with the first being the initial reason I opened this PR.

  1. Metadata table was not balancing
  2. Sometimes user tables would balance when the metadata table was not balanced
  3. Each call to the balancer would balance all tables. For example when balancing the root table it would compute balancing for all tables.

For 1 the changes in 6eab50f fix this. For 2 and 3 the changes in 6de5ef9 fix these. For 2 created a new Migrations class that tracks migrations per data level so that when making balancing decisions it could always know if the previous levels had migrations. For 3 added a new method to the balancer SPI that conveys what tables need to be balanced. This new method support partitioning tables for balancing in different ways and allows balancers to delegate to other balancers with smaller partitions. May also allow different manager processes to parition tables for balancing in the future.

Took this out of draft after the changes in 6de5ef9

The new IT added only test problem 1. For problems 2 and 3, I verified those by running the IT and looking at the manager logs. The test exercises the cause of problems 2 and 3 but will not fail if they occur.

@keith-turner keith-turner changed the title Fixes balance related filtering that was using table name instead of id Fixes balancing of metadata table Dec 21, 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.

2 participants