You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2025/05/22 16:07:32 UTC
[PR] Fixed issue with balancer property updates [accumulo]
dlmarion opened a new pull request, #5575:
URL: https://github.com/apache/accumulo/pull/5575
The change in #5530 added code to clear the
property cache in initializeBalancer because
the property change was not immediately seen.
BrokenBalancerIT was changing the property
then checking the balance to see if the new
balancer had taken effect.
This commit changes BrokenBalancerIT to give
it more time in the test thread for the
property change to take effect in the Manager
and changes how the Manager responds to system
property changes so that the property cache
invalidation can be removed.
Related to #5530
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#issuecomment-2904772045
Implemented suggestion in 7acff53. I had to increase the wait time in BrokenBalancerIT for the Manager to pick up the property change as the DoNothingBalancer returns 30s. I'm going to kick off a full IT build to see if this change causes other balancing related ITs to fail.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#issuecomment-2901802155
This change was based on the information found in this [thread](https://github.com/apache/accumulo/pull/5530/files#r2102684523)
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#issuecomment-2904431427
> I'm not sure why the balancer code needs to register a listener, or do anything to try to respond in near real time to config changes. Why not just poll the relevant propstore to see if anything has changed before each balance attempt? Those are pretty infrequent, and polling the PropStore, or even reading the relevant properties out of the cached system config snapshot to detect a change, should be very fast.
I like that approach. Seems like it would remove the specialized code and make the code easier to maintain w/o any penalty.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#discussion_r2103000346
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -425,6 +428,7 @@ protected Manager(ServerOpts opts, String[] args) throws IOException {
log.info("Instance {}", getInstanceID());
timeKeeper = new ManagerTime(this, aconf);
tserverSet = new LiveTServerSet(context, this);
+ context.getPropStore().registerAsListener(SystemPropKey.of(context), this);
Review Comment:
The impl of `registerAsListener` will add the listener to a set, so the order in which listeners are called will vary at runtime. I am not sure, but it seems like maybe there is another listener that invalidates the prop store cache. If this is the case this the listener registered here could execute before or after the listener that clears the cache depending on their order in the hash set. That could cause this listener to sometime see config change and sometimes not. However this may not be correct, drawing this conclusion base on looking at some of the code. If it is the case maybe a LinkedHashSet could be used to make the order in which listeners are called correspond with the order they were added.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#discussion_r2109767959
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1917,19 +1920,22 @@ public boolean isUpgrading() {
return upgradeCoordinator.getStatus() != UpgradeCoordinator.UpgradeStatus.COMPLETE;
}
- void initializeBalancer() {
+ private void initializeBalancer() {
+ String configuredBalancerClass = getConfiguration().get(Property.MANAGER_TABLET_BALANCER);
try {
- getContext().getPropStore().getCache().removeAll();
- getConfiguration().invalidateCache();
- log.debug("Attempting to reinitialize balancer using class {}",
- getConfiguration().get(Property.MANAGER_TABLET_BALANCER));
- var localTabletBalancer = Property.createInstanceFromPropertyName(getConfiguration(),
- Property.MANAGER_TABLET_BALANCER, TabletBalancer.class, new DoNothingBalancer());
- localTabletBalancer.init(balancerEnvironment);
- tabletBalancer = localTabletBalancer;
+ if (tabletBalancer == null
+ || !tabletBalancer.getClass().getName().equals(configuredBalancerClass)) {
+ log.debug("Attempting to initialize balancer using class {}, was {}",
+ configuredBalancerClass,
+ tabletBalancer == null ? "null" : tabletBalancer.getClass().getName());
+ var localTabletBalancer = Property.createInstanceFromPropertyName(getConfiguration(),
+ Property.MANAGER_TABLET_BALANCER, TabletBalancer.class, new DoNothingBalancer());
+ localTabletBalancer.init(balancerEnvironment);
+ tabletBalancer = localTabletBalancer;
Review Comment:
Added log stmt in fcd25da
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#issuecomment-2912273622
Full IT build successful
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#discussion_r2109371093
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1917,19 +1920,22 @@ public boolean isUpgrading() {
return upgradeCoordinator.getStatus() != UpgradeCoordinator.UpgradeStatus.COMPLETE;
}
- void initializeBalancer() {
+ private void initializeBalancer() {
+ String configuredBalancerClass = getConfiguration().get(Property.MANAGER_TABLET_BALANCER);
try {
- getContext().getPropStore().getCache().removeAll();
- getConfiguration().invalidateCache();
- log.debug("Attempting to reinitialize balancer using class {}",
- getConfiguration().get(Property.MANAGER_TABLET_BALANCER));
- var localTabletBalancer = Property.createInstanceFromPropertyName(getConfiguration(),
- Property.MANAGER_TABLET_BALANCER, TabletBalancer.class, new DoNothingBalancer());
- localTabletBalancer.init(balancerEnvironment);
- tabletBalancer = localTabletBalancer;
+ if (tabletBalancer == null
+ || !tabletBalancer.getClass().getName().equals(configuredBalancerClass)) {
+ log.debug("Attempting to initialize balancer using class {}, was {}",
+ configuredBalancerClass,
+ tabletBalancer == null ? "null" : tabletBalancer.getClass().getName());
+ var localTabletBalancer = Property.createInstanceFromPropertyName(getConfiguration(),
+ Property.MANAGER_TABLET_BALANCER, TabletBalancer.class, new DoNothingBalancer());
+ localTabletBalancer.init(balancerEnvironment);
+ tabletBalancer = localTabletBalancer;
Review Comment:
There used to be the following log message from ManagerClientService.updatePlugins that logged something at info once the change was complete. Would be nice to still have that. The following probably will not compile, copied the log message exactly from the old place in the code.
```suggestion
tabletBalancer = localTabletBalancer;
log.info("tablet balancer changed to {}", manager.getBalancerClass().getName());
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java:
##########
@@ -591,15 +584,6 @@ 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());
Review Comment:
Is the getBalancerClass() method in the manager still used by other stuff? Wondering if it could be removed.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#discussion_r2109768598
##########
server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java:
##########
@@ -591,15 +584,6 @@ 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());
Review Comment:
It was not being used anymore, removed it in fcd25da.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #5575:
URL: https://github.com/apache/accumulo/pull/5575
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#discussion_r2103000346
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -425,6 +428,7 @@ protected Manager(ServerOpts opts, String[] args) throws IOException {
log.info("Instance {}", getInstanceID());
timeKeeper = new ManagerTime(this, aconf);
tserverSet = new LiveTServerSet(context, this);
+ context.getPropStore().registerAsListener(SystemPropKey.of(context), this);
Review Comment:
The impl of `registerAsListener` will add the listener to a set, so the order in which listeners are called will vary at runtime. I am not sure, but it seems like maybe there is another listener that invalidates the prop store cache. If this is the case this the listener registered here could execute before or after the listener that clears the cache depending on their order in the hash set. That could cause this listener to sometime see config change and sometimes not. However this analysis may not be correct, drawing this conclusion based on looking at some of the code. If it is the case maybe a LinkedHashSet could be used to make the order in which listeners are called correspond with the order they were added.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#discussion_r2103000346
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -425,6 +428,7 @@ protected Manager(ServerOpts opts, String[] args) throws IOException {
log.info("Instance {}", getInstanceID());
timeKeeper = new ManagerTime(this, aconf);
tserverSet = new LiveTServerSet(context, this);
+ context.getPropStore().registerAsListener(SystemPropKey.of(context), this);
Review Comment:
The impl of `registerAsListener` will add the listener to a set, so the order in which listeners are called will vary at runtime. I am not sure, but it seems like maybe there is another listener that invalidates the prop store cache. If this is the case this the listener registered here could execute before or after the listener that clears the cache depending on their order in the hash set. That could cause this listener to sometime see config change and sometime now. However this may not be correct, drawing this conclusion base on looking at some of the code. If it is the case maybe a LinkedHashSet could be used to make the order in which listeners are called correspond with the order they were added.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fixed issue with balancer property updates [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5575:
URL: https://github.com/apache/accumulo/pull/5575#discussion_r2103098577
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -425,6 +428,7 @@ protected Manager(ServerOpts opts, String[] args) throws IOException {
log.info("Instance {}", getInstanceID());
timeKeeper = new ManagerTime(this, aconf);
tserverSet = new LiveTServerSet(context, this);
+ context.getPropStore().registerAsListener(SystemPropKey.of(context), this);
Review Comment:
`ZooPropStore.connectionEvent` does call `PropCache.removeAll`. I don't really see any other places where the cache is invalidated.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org