imbajin commented on code in PR #3418:
URL: https://github.com/apache/hugegraph/pull/3418#discussion_r4397192853
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/TaskScheduleService.java:
##########
@@ -449,22 +450,61 @@ public synchronized Map<Integer, KVPair<Long, Long>>
balancePartitionShard() thr
maxHeap.addAll(tmpList);
}
- kvService.put(BALANCE_SHARD_KEY, "DOING", 180 * 1000);
-
- // Start the migration
- movedPartitions.forEach((partId, storePair) -> {
- // Neither the source nor destination storeID is 0
- if (storePair.getKey() > 0 && storePair.getValue() > 0) {
- partitionService.movePartitionsShard(partId,
storePair.getKey(),
- storePair.getValue());
- } else {
+ boolean submitted = false;
+ Iterator<Map.Entry<Integer, KVPair<Long, Long>>> submission =
+ movedPartitions.entrySet().iterator();
+ while (submission.hasNext()) {
+ Map.Entry<Integer, KVPair<Long, Long>> move = submission.next();
+ KVPair<Long, Long> stores = move.getValue();
+ if (stores.getKey() <= 0 || stores.getValue() <= 0) {
log.warn("balancePartitionShard key or value is zero,
partId:{} storePair:{}",
- partId, storePair);
+ move.getKey(), stores);
+ submission.remove();
+ continue;
}
- });
+ if (!movePartitionIfPlacementStore(move.getKey(), stores.getKey(),
+ stores.getValue())) {
+ submission.remove();
+ continue;
+ }
+ submitted = true;
+ }
+ if (submitted) {
+ kvService.put(BALANCE_SHARD_KEY, "DOING", 180 * 1000);
+ }
return movedPartitions;
}
+ /**
+ * Revalidate destination liveness and state while serializing with Store
state changes.
+ * Preserve the existing service lock order: PartitionService then
StoreNodeService.
+ */
+ private boolean movePartitionIfPlacementStore(int partitionId, long
sourceStoreId,
+ long destStoreId) {
+ synchronized (partitionService) {
+ synchronized (storeService) {
Review Comment:
**[P1] Remove the remaining placement-versus-split lock inversion**
Moving the Tombstone callback outside the Store monitor fixes that reported
path, but the two monitors here still oppose the manual split path:
* This helper holds `partitionService` at line 484 and then acquires
`storeService` at line 485.
*
[`TaskScheduleService.splitPartition()`](https://github.com/apache/hugegraph/blob/50515cb821c0ce1afd5ec3fc373104431cb7a881/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/TaskScheduleService.java#L649-L660)
is not synchronized and calls
[`StoreNodeService.splitShardGroups()`](https://github.com/apache/hugegraph/blob/50515cb821c0ce1afd5ec3fc373104431cb7a881/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/StoreNodeService.java#L617-L626),
which holds the Store monitor while calling
[`PartitionService.splitPartition()`](https://github.com/apache/hugegraph/blob/50515cb821c0ce1afd5ec3fc373104431cb7a881/hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/PartitionService.java#L630-L645),
synchronized on the Partition monitor.
Consequently, concurrent placement and a split that passes its capacity
check can each own one monitor and wait indefinitely for the other. The
scheduler's synchronized balancing entry point does not exclude the
unsynchronized split entry point. This can block both services, including Store
heartbeat/state operations, rather than just one migration.
Please establish a consistent lock order across these paths while retaining
the final destination-state check. The new Tombstone-listener regression does
not exercise this remaining split interleaving; add a controlled
split-versus-placement regression that demonstrates both operations complete
without opposite monitor acquisition.
This is distinct from the now-fixed Tombstone callback cycle in [the earlier
comment](https://github.com/apache/hugegraph/pull/3418#discussion_r4395010891).
Validation here is static source tracing at
`50515cb821c0ce1afd5ec3fc373104431cb7a881`, not an independently executed
deadlock test or a data-loss claim.
--
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]
