kevinrr888 commented on code in PR #5537:
URL: https://github.com/apache/accumulo/pull/5537#discussion_r2140528929
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -376,7 +360,7 @@ private int nonMetaDataTabletsAssignedOrHosted() {
- assignedOrHosted(SystemTables.ROOT.tableId());
}
- private int notHosted() {
+ int notHosted() {
Review Comment:
Could move this method to BalanceManager
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -580,7 +559,7 @@ public void run() {
}
}
- private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
Review Comment:
Could potentially move the migration cleanup thread into BalanceManager (and
move this method as well)
##########
server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java:
##########
@@ -555,17 +553,12 @@ private void alterTableProperty(TCredentials c, String
tableName, String propert
}
private void updatePlugins(String property) {
- // resolve without warning; any warnings should have already occurred
- String resolved = DeprecatedPropertyUtil.getReplacementName(property,
(log, replacement) -> {});
- if (resolved.equals(Property.MANAGER_TABLET_BALANCER.getKey())) {
- manager.initializeBalancer();
- log.info("tablet balancer changed to {}",
manager.getBalancerClass().getName());
- }
+ manager.getBalanceManager().propertyChanged(property);
Review Comment:
`String resolved = ...` and the log seem to have been dropped. Was this
intentional?
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -452,15 +436,14 @@ protected Manager(ConfigOpts opts,
Function<SiteConfiguration,ServerContext> ser
super(ServerId.Type.MANAGER, opts, serverContextFactory, args);
ServerContext context = super.getContext();
upgradeCoordinator = new UpgradeCoordinator(context);
- balancerEnvironment = new BalancerEnvironmentImpl(context);
+ balanceManager = new BalanceManager(this);
AccumuloConfiguration aconf = context.getConfiguration();
log.info("Version {}", Constants.VERSION);
log.info("Instance {}", context.getInstanceID());
timeKeeper = new ManagerTime(this, aconf);
tserverSet = new LiveTServerSet(context, this);
Review Comment:
Just noticed the new `BalanceManager` as well as the existing `ManagerTime`
and `LiverTServerSet` are taking `this` before `Manager` is fully constructed
and want to ensure these use cases are safe
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1818,44 +1586,6 @@ public boolean isUpgrading() {
return upgradeCoordinator.getStatus() !=
UpgradeCoordinator.UpgradeStatus.COMPLETE;
}
- void initializeBalancer() {
- var localTabletBalancer =
Property.createInstanceFromPropertyName(getConfiguration(),
- Property.MANAGER_TABLET_BALANCER, TabletBalancer.class, new
TableLoadBalancer());
- localTabletBalancer.init(balancerEnvironment);
- tabletBalancer = localTabletBalancer;
- }
-
- Class<?> getBalancerClass() {
- return tabletBalancer.getClass();
- }
-
- void getAssignments(SortedMap<TServerInstance,TabletServerStatus>
currentStatus,
- Map<String,Set<TServerInstance>> currentTServerGroups,
- Map<KeyExtent,UnassignedTablet> unassigned,
Map<KeyExtent,TServerInstance> assignedOut) {
- AssignmentParamsImpl params =
- AssignmentParamsImpl.fromThrift(currentStatus, currentTServerGroups,
- unassigned.entrySet().stream().collect(HashMap::new,
- (m, e) -> m.put(e.getKey(),
- e.getValue().getLastLocation() == null ? null
- : e.getValue().getLastLocation().getServerInstance()),
- Map::putAll),
- assignedOut);
- tabletBalancer.getAssignments(params);
- }
-
- public TabletStateStore getTabletStateStore(DataLevel level) {
Review Comment:
Was this unused?
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1873,7 +1603,7 @@ public ServiceLock getLock() {
return managerLock;
}
- private long numMigrations() {
+ long numMigrations() {
Review Comment:
Could also move this method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
