bitflicker64 commented on code in PR #3418:
URL: https://github.com/apache/hugegraph/pull/3418#discussion_r4395010891
##########
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:
Important: `updateStore()` holds the `StoreNodeService` monitor while its
Tombstone listener enters `PartitionService.shardOffline()`, so taking
`partitionService` then `storeService` here deadlocks `movePartitions()` or
`balancePartitionShard()` against a concurrent Tombstone `setStore` request. A
focused two-thread test reproduces the cycle. Take the two monitors in one
order on every path, for example by firing `onStoreStatusChanged` after
`updateStore()` releases its lock.
--
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]
