Repository navigation
Conversation
- Separate live Stores from Up placement destinations using current metadata. - Keep Exiting leases alive until local engines and PD references are empty. - Serialize heartbeat, registration and state changes and preserve drain on restart. - Cover drain, stale leases, shared groups and concurrent updates in regressions.
| } | ||
|
|
||
| int activeStores = storeService.getActiveStores().size(); | ||
| List<Metapb.Store> placementStores = storeService.getPlacementStores(); |
There was a problem hiding this comment.
[P2] Fence migration submission against destination state changes
This filters destinations when planning, but the list is only a snapshot. A destination can become Exiting after this read and before the migration is submitted. movePartitionsShard() appends toStore as a follower and sends ChangeShard without checking its current state.
A deterministic regression can pause balancing after selecting an Up destination, complete that destination's transition to Exiting on another thread, then resume submission. The old plan should not send a replica to that Store. The synchronized methods here, in PartitionService, and in StoreNodeService lock different objects, so they do not establish this ordering.
Please make the final eligibility check and migration submission use a consistent synchronization/state-version contract with Store state transitions. An additional unprotected list read still leaves the check-to-use window.
This is pre-existing incomplete protection, not a newly introduced P1: the baseline also planned from a snapshot and used the unchecked submission path. It is separate from the blocked-drain retry finding. The new filtering fixes stale-state selection at planning time but does not fully establish the documented exclusion of draining destinations.
Validation: independent static cross-check at b84bf81b8962507027d811206d8fa8a590e8b9d0; no live-cluster race reproduction or data-loss claim. Related source-PR observation: 🔗 hugegraph/hugegraph #343.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3418 +/- ##
============================================
+ Coverage 57.27% 57.73% +0.46%
+ Complexity 6577 2875 -3702
============================================
Files 776 776
Lines 68881 68972 +91
Branches 9431 9456 +25
============================================
+ Hits 39449 39822 +373
+ Misses 25804 25499 -305
- Partials 3628 3651 +23
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The drain-state changes hold up, but the nested monitors in movePartitionIfPlacementStore() can deadlock PD against a concurrent Tombstone update. The earlier comment on StoreNodeService.java line 521 about resuming a blocked drain still applies at 30db68f.
Retry explicit Exiting requests after eligible capacity is restored. Dispatch management state notifications after releasing the Store lock. Preserve Tombstone as terminal across management state updates. Cover capacity recovery and concurrent placement with regressions.
| private boolean movePartitionIfPlacementStore(int partitionId, long sourceStoreId, | ||
| long destStoreId) { | ||
| synchronized (partitionService) { | ||
| synchronized (storeService) { |
There was a problem hiding this 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
partitionServiceat line 484 and then acquiresstoreServiceat line 485. TaskScheduleService.splitPartition()is not synchronized and callsStoreNodeService.splitShardGroups(), which holds the Store monitor while callingPartitionService.splitPartition(), 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. Validation here is static source tracing at 50515cb821c0ce1afd5ec3fc373104431cb7a881, not an independently executed deadlock test or a data-loss claim.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The drain-state, lease and placement changes hold up, but the PR ships two one-off contents: write workflows that commit and push to fork branches. The open split-versus-placement lock inversion comment on TaskScheduleService.java line 485 still applies at a85d49f.
| - .github/workflows/fix-pr343-duplicate-drain.yml | ||
|
|
||
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
Important: this file and stage-pr343-tested-fix.yml are one-off branch automation with contents: write that patches PDService.java, commits as chatgpt-codex-connector[bot] and pushes to codex/* branches; merging adds them to apache/hugegraph, and the PDService change they carry is not part of this PR. Remove both workflow files and commit any intended duplicate-drain fix as source.
Remove the two branch-specific patch and staging workflows.\nKeep regression tests and production changes in their normal modules.\nRetain validation evidence outside the source tree.
Before → After
Before: an Exiting Store could become Tombstone on lease expiry while still owning partitions; a stale Up lease could also make placement select it again.
After: separate draining liveness from placement eligibility, renew the drain lease, and require zero local engines plus no PD shard references before retirement. Serialize heartbeat/registration with state changes and preserve Exiting on re-registration.
Validation boundary
The source PR reports 11 drain regressions and 29 existing service tests, plus formatting and a clean compile. Baseline combined-test failures are documented in the source PR. These reports are not independent cluster-level retirement acceptance.
Source PR: 🔗 hugegraph/hugegraph #343
Visual overview
Drain-state and placement checks are distinct from liveness. The current split-versus-placement lock-order inversion remains unresolved; this diagram does not establish complete drain recovery or RC acceptance.