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 2026/05/18 19:02:31 UTC
[PR] Use per-table merge lock in Manager instead of single lock [accumulo]
dlmarion opened a new pull request, #6378:
URL: https://github.com/apache/accumulo/pull/6378
This change introduces a per-table merge lock instead of a single lock to protect reading/writing to ZooKeeper.
Closes #6374
--
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] Use per-table merge lock in Manager instead of single lock [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6378:
URL: https://github.com/apache/accumulo/pull/6378#discussion_r3262357458
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1479,6 +1514,9 @@ boolean canSuspendTablets() {
throw new IllegalStateException("Exception updating manager lock", e);
}
+ ThreadPools.watchCriticalScheduledTask(
+ context.getScheduledExecutor().scheduleWithFixedDelay(mergeLocks::cleanup, 3, 3, HOURS));
+
Review Comment:
No need for this cleanup code with the weakValues cache's scheduler.
--
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] Use per-table merge lock in Manager instead of single lock [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #6378:
URL: https://github.com/apache/accumulo/pull/6378#discussion_r3282953557
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -496,15 +505,19 @@ public MergeInfo getMergeInfo(TableId tableId) {
log.warn("Unexpected error reading merge state", ex);
return new MergeInfo();
}
+ } finally {
+ l.unlock();
}
}
public void setMergeState(MergeInfo info, MergeState state)
throws KeeperException, InterruptedException {
ServerContext context = getContext();
- synchronized (mergeLock) {
- String path =
- getZooKeeperRoot() + Constants.ZTABLES + "/" + info.getExtent().tableId() + "/merge";
+ final TableId tid = info.getExtent().tableId();
Review Comment:
This code looks like we might hit a NPE on the `.tableId()` call if the extent doesn't exist.
MergeInfo has a no-args constructor that does not set the extent.
Instead the extent is then set via a `MergeInfo.readFields()` call.
However for all calls of `setMergeState` the extent or state of `MergeInfo` is checked so this doesn't seem to be an issue.
--
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] Use per-table merge lock in Manager instead of single lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #6378:
URL: https://github.com/apache/accumulo/pull/6378
--
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] Use per-table merge lock in Manager instead of single lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6378:
URL: https://github.com/apache/accumulo/pull/6378#discussion_r3320431816
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -496,15 +505,19 @@ public MergeInfo getMergeInfo(TableId tableId) {
log.warn("Unexpected error reading merge state", ex);
return new MergeInfo();
}
+ } finally {
+ l.unlock();
}
}
public void setMergeState(MergeInfo info, MergeState state)
throws KeeperException, InterruptedException {
ServerContext context = getContext();
- synchronized (mergeLock) {
- String path =
- getZooKeeperRoot() + Constants.ZTABLES + "/" + info.getExtent().tableId() + "/merge";
+ final TableId tid = info.getExtent().tableId();
Review Comment:
The code prior to this change used the same method call with no NPE checking, and I think without issue. I think this is safe.
--
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] Use per-table merge lock in Manager instead of single lock [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6378:
URL: https://github.com/apache/accumulo/pull/6378#discussion_r3262338172
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -201,8 +226,8 @@ public class Manager extends AbstractServer implements LiveTServerSet.Listener,
Collections.synchronizedMap(new HashMap<>());
final Set<TServerInstance> serversToShutdown = Collections.synchronizedSet(new HashSet<>());
final Migrations migrations = new Migrations();
+ private final MergeLocks mergeLocks = new MergeLocks();
Review Comment:
```suggestion
private final LoadingCache<TableId,ReentrantLock> mergeLocks = Caffeine.newBuilder().weakValues()
.scheduler(Scheduler.systemScheduler()).build(k -> new ReentrantLock());
```
My main concern with this is that the TableId in the keys will prevent cleanup of the TableId.cache weak values. The scheduler is supposed to help with cleanup of the keys whose values have been garbage collected, but it may also be a good idea to use String (`tableId.canonical()`) for the key.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -496,15 +523,19 @@ public MergeInfo getMergeInfo(TableId tableId) {
log.warn("Unexpected error reading merge state", ex);
return new MergeInfo();
}
+ } finally {
+ l.unlock();
}
}
public void setMergeState(MergeInfo info, MergeState state)
throws KeeperException, InterruptedException {
ServerContext context = getContext();
- synchronized (mergeLock) {
- String path =
- getZooKeeperRoot() + Constants.ZTABLES + "/" + info.getExtent().tableId() + "/merge";
+ final TableId tid = info.getExtent().tableId();
+ final ReentrantLock l = mergeLocks.getLock(tid);
Review Comment:
```suggestion
final ReentrantLock l = mergeLocks.get(tid);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -477,7 +502,9 @@ public TServerConnection getConnection(TServerInstance server) {
public MergeInfo getMergeInfo(TableId tableId) {
ServerContext context = getContext();
- synchronized (mergeLock) {
+ final ReentrantLock l = mergeLocks.getLock(tableId);
Review Comment:
```suggestion
final ReentrantLock l = mergeLocks.get(tableId);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -519,16 +550,20 @@ public void setMergeState(MergeInfo info, MergeState state)
state.equals(MergeState.STARTED) ? ZooUtil.NodeExistsPolicy.FAIL
: ZooUtil.NodeExistsPolicy.OVERWRITE);
}
- mergeLock.notifyAll();
+ } finally {
+ l.unlock();
}
nextEvent.event("Merge state of %s set to %s", info.getExtent(), state);
}
public void clearMergeState(TableId tableId) throws KeeperException, InterruptedException {
- synchronized (mergeLock) {
+ final ReentrantLock l = mergeLocks.getLock(tableId);
Review Comment:
```suggestion
final ReentrantLock l = mergeLocks.get(tableId);
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1479,6 +1514,9 @@ boolean canSuspendTablets() {
throw new IllegalStateException("Exception updating manager lock", e);
}
+ ThreadPools.watchCriticalScheduledTask(
+ context.getScheduledExecutor().scheduleWithFixedDelay(mergeLocks::cleanup, 3, 3, HOURS));
+
Review Comment:
No need for this cleanup code with the weakValues cache's scheduler.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -172,6 +173,30 @@
public class Manager extends AbstractServer implements LiveTServerSet.Listener, TableObserver,
CurrentState, HighlyAvailableService, ServerProcessService.Iface {
+ private final class MergeLocks {
+
+ private final Object lock = new Object();
+ private Map<TableId,ReentrantLock> lockStorage = new HashMap<TableId,ReentrantLock>();
+
+ private ReentrantLock getLock(TableId tid) {
+ synchronized (lock) {
+ return lockStorage.computeIfAbsent(tid, k -> new ReentrantLock(true));
+ }
+ }
+
+ private void cleanup() {
+ synchronized (lock) {
+ Set<TableId> removals = new HashSet<>();
+ for (Entry<TableId,ReentrantLock> e : lockStorage.entrySet()) {
+ if (!getContext().tableNodeExists(e.getKey()) && !e.getValue().isLocked()) {
+ removals.add(e.getKey());
+ }
+ }
+ removals.forEach(lockStorage::remove);
+ }
+ }
+ }
+
Review Comment:
```suggestion
```
--
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] Use per-table merge lock in Manager instead of single lock [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6378:
URL: https://github.com/apache/accumulo/pull/6378#discussion_r3262357458
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1479,6 +1514,9 @@ boolean canSuspendTablets() {
throw new IllegalStateException("Exception updating manager lock", e);
}
+ ThreadPools.watchCriticalScheduledTask(
+ context.getScheduledExecutor().scheduleWithFixedDelay(mergeLocks::cleanup, 3, 3, HOURS));
+
Review Comment:
No need for this cleanup code with the weakValues cache's scheduler.
```suggestion
```
--
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] Use per-table merge lock in Manager instead of single lock [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #6378:
URL: https://github.com/apache/accumulo/pull/6378#issuecomment-4575585806
These changes were ignored on the merge to main as merging is done differently in the main branch
--
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