Moves balancing code out of Manager class #5537
Conversation
The balancing code needs some improvements. Currently the code is entangled in the manager code in odd places (like under some of the balancing code is un the Manager.StatusThread class). This change moves the balancing code to its own class w/o making any changes to the code (except fixing two small existing bugs in shouldCleanupMigration()). Making this change as a first step in making the balancer more independent of other manager code and threads.
| try ( | ||
| var tabletsMetadata = getContext() | ||
| .getAmple().readTablets().forLevel(dl).fetch(TabletMetadata.ColumnType.PREV_ROW, | ||
| TabletMetadata.ColumnType.LOCATION, TabletMetadata.ColumnType.MIGRATION) |
There was a problem hiding this comment.
Added the LOCATION column to this code because I noticed an error in the logs where shouldCleanupMigration() was throw an exception because the column was not fetched.
| balancedNotifier.notifyAll(); | ||
| } | ||
| } else if (totalMigrationsOut > 0) { | ||
| manager.nextEvent.event("Migrating %d more tablets, %d total", totalMigrationsOut, |
There was a problem hiding this comment.
All of the code in the class was copied as is from the manager and then some small changes were made to add the manager. prefix here. Did not change the functionality of any of the code though.
| var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId()); | ||
| var migration = tabletMetadata.getMigration(); | ||
| Preconditions.checkState(migration != null, | ||
| "This method should only be called if there is a migration"); | ||
| return tableState == TableState.OFFLINE || !onlineTabletServers().contains(migration) | ||
| || tabletMetadata.getLocation().getServerInstance().equals(migration); | ||
| || (tabletMetadata.getLocation() != null |
There was a problem hiding this comment.
This is the 2nd existing bug that was fixed in this change (the first was fetching the location column when calling this). Was seeing an NPE here in the logs.
| } | ||
|
|
||
| private void initializeBalancer() { | ||
| var localTabletBalancer = |
There was a problem hiding this comment.
We should probably log which balancer class was created
There was a problem hiding this comment.
updated in 705d58b
| } | ||
|
|
||
| void propertyChanged(String property) { | ||
| String resolved = DeprecatedPropertyUtil.getReplacementName(property, (log, replacement) -> {}); |
There was a problem hiding this comment.
Is there still an old deprecated property in 4.0? If so, should we be looking for the deprecated property in initializeBalancer?
There was a problem hiding this comment.
This code is probably no longer needed.
There was a problem hiding this comment.
removed in 705d58b
I think that is the problem that needs the balancer to run in a separate thread. With this change was just attempting to clean up some tech debt in the way the balancer code is organized before making the threading changes. Hoping we can make a change to spin up three balancer threads and remove balancing from the status thread after this change. |
The balancing code needs some improvements. Currently the code is entangled in the manager code in odd places (like some of the balancing code is un the Manager.StatusThread class). This change moves the balancing code to its own class w/o making any changes to the code (except fixing two small existing bugs in shouldCleanupMigration()).
Making this change as a first step in making the balancer more independent of other manager code and threads.