bitflicker64 commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4027644539
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1413,30 +1456,58 @@ public boolean dbCompaction(String graphName, int id,
String tableName) {
pathLock.putIfAbsent(path, new
AtomicInteger(compactionCanStart));
compactionState.putIfAbsent(id, new AtomicInteger(0));
log.info("Partition {} dbCompaction started", id);
- if (tableName.isEmpty()) {
- lock(path);
- setState(id, doing);
- log.info("Partition {}-{} got lock, dbCompaction
start", id, path);
- op.compactRange();
- setState(id, compactionDone);
- log.info("Partition {} dbCompaction end and start
to do snapshot", id);
- PartitionEngine pe =
HgStoreEngine.getInstance().getPartitionEngine(id);
- // find leader and send blankTask, after execution
- if (pe.isLeader()) {
- RaftClosure bc = (closure) -> {
- };
-
pe.addRaftTask(RaftOperation.create(RaftOperation.SYNC_BLANK_TASK),
- bc);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(id,
+ k -> new
ReentrantLock());
+ boolean rangeLocked = false;
+ try {
+ rangeLocked =
rangeLock.tryLock(compactionRangeLockWaitMillis,
+
TimeUnit.MILLISECONDS);
+ if (rangeLocked) {
+ if (tableName.isEmpty()) {
+ lock(path);
+ setState(id, doing);
+ log.info("Partition {}-{} got lock,
dbCompaction start", id, path);
+ op.compactRange();
+ setState(id, compactionDone);
+ log.info("Partition {} dbCompaction end
and start to do snapshot", id);
+ PartitionEngine pe =
HgStoreEngine.getInstance().getPartitionEngine(id);
+ // find leader and send blankTask, after
execution
+ if (pe.isLeader()) {
+ RaftClosure bc = (closure) -> {
+ };
+
pe.addRaftTask(RaftOperation.create(RaftOperation.SYNC_BLANK_TASK),
+ bc);
+ } else {
+ HgCmdClient client =
HgStoreEngine.getInstance().getHgCmdClient();
+ BlankTaskRequest request = new
BlankTaskRequest();
+ request.setGraphName("");
+ request.setPartitionId(id);
+
client.tryInternalCallSyncWithRpc(request);
+ }
+ setAndNotifyState(id, compactionDone);
+ } else {
+ op.compactRange(tableName);
+ }
} else {
- HgCmdClient client =
HgStoreEngine.getInstance().getHgCmdClient();
- BlankTaskRequest request = new
BlankTaskRequest();
- request.setGraphName("");
- request.setPartitionId(id);
- client.tryInternalCallSyncWithRpc(request);
+ log.warn("Partition {} skip dbCompaction,
snapshot save " +
+ "still in progress after {}ms wait",
id,
+ compactionRangeLockWaitMillis);
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ log.warn("Partition {} dbCompaction interrupted
while waiting " +
+ "for snapshot range lock", id);
+ // Interrupted while waiting for the snapshot save
to release
+ // the range lock. The path lock was already
acquired and
+ // must be released here, otherwise later
compactions for this
+ // partition would block until the path lock
timeout.
+ unlock(path);
Review Comment:
⚠️ Important. The last commit moved `rangeLock.tryLock(...)` in front of
`lock(path)` (in 54d4fe5 it came after). That causes two problems.
1. This `unlock(path)` now runs when the task never held the path lock: an
interrupt in `tryLock` at 1464, or in `lock(path)` itself, or any interrupt on
the `tableName` branch, which never takes it at all. `unlock` sets the shared
`AtomicInteger` back to `compactionCanStart`. If an earlier compaction on this
partition still holds it (it stays held until `PartitionEngine.doSnapshotSync`
finishes), that lock gets cleared and a third compaction can start before the
earlier post-compaction snapshot is done. The comment above says "the path lock
was already acquired", which is no longer true.
`testDbCompactionReleasesPathLockWhenInterruptedWaitingForRangeLock` still
passes only because the path lock was never set to `doing` in that test.
2. The range lock is now held while `lock(path)` waits, which can be up to
`timeoutMillis` (6h), and while the blank-task RPC runs. When a second
`dbCompaction` for the same partition arrives while the first one's
post-compaction snapshot is pending (the callers at
PartitionEngine.java:1018/1266, HgStoreEngine.java:505 and the TTL submitter
don't go through the semaphore), it takes the range lock and blocks in
`lock(path)`. The snapshot that would release `path` then calls
`tryLockCompactionRange`, gets EBUSY, and every periodic snapshot on the
partition fails the same way until the wait ends.
Requested change: go back to taking `lock(path)` first, hold the range lock
only around `op.compactRange()` / `op.compactRange(tableName)`, and call
`unlock(path)` only when `lock(path)` actually succeeded (for example with a
`pathLocked` flag). Please also update the test so the path lock is really in
`doing` before the interrupt.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandlerImpl.java:
##########
@@ -1190,15 +1212,36 @@ public boolean cleanPartition(String graph, int partId,
long startKey, long endK
taskManager.putAsyncTask(cleanTask);
Utils.runInThread(() -> {
- cleanPartition(partition, code -> {
- // in range
- boolean flag = code >= startKey && code < endKey;
- return (cleanType == CleanType.CLEAN_TYPE_KEEP_RANGE) == flag;
- });
- // May have been destroyed.
- if (HgStoreEngine.getInstance().getPartitionEngine(partId) !=
null) {
- taskManager.updateAsyncTaskState(partId, graph,
cleanTask.getId(),
- AsyncTaskState.SUCCESS);
+ ReentrantLock rangeLock =
+ compactionRangeLock.computeIfAbsent(partId, k -> new
ReentrantLock());
+ boolean rangeLocked = false;
+ try {
+ rangeLocked = rangeLock.tryLock(compactionRangeLockWaitMillis,
Review Comment:
⚠️ Important. This lock is now held for the entire
`cleanPartition(partition, ...)` call. That call scans every key in the
partition and deletes the ones out of range before it reaches
`op.compactRange()` at line 1310. On a large partition that can take minutes.
For all of that time every `onSnapshotSave` for the partition fails with EBUSY,
so the raft log isn't truncated, and any `dbCompaction` on the partition waits
10s and is then dropped. The field comment at 168-178 assumes the lock is held
for under a second.
The skip branch also has a cost: if a snapshot save holds the lock past 10s,
the whole cleanup is dropped, not just the compaction. This is the one-shot
cleanup after a split or move (DataManagerImpl.java:246,
DefaultDataMover.java:245), so the data that no longer belongs to this
partition stays, and the CleanTask is left in START.
Requested change: take the range lock only around the final
`op.compactRange()` inside the private `cleanPartition(Partition, Function)`,
with the same bounded wait, and let the delete pass run unlocked. If the
compaction step times out, log it but keep the cleanup.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
