Repository navigation
fix(pd): clean deleted graph-space statistics - #3403
Conversation
Remove the exact PD quota and usage record when Server deletes a space. Prevent older quota snapshots from recreating deleted records or graphs. Propagate PD graph cleanup failures so Server can retain owners for retry. Cover empty spaces, deletion races, idempotency and older-PD RPC failures.
…pace-statistics-cleanup
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3403 +/- ##
============================================
+ Coverage 55.93% 57.22% +1.29%
- Complexity 2277 2805 +528
============================================
Files 774 776 +2
Lines 68534 68967 +433
Branches 9333 9434 +101
============================================
+ Hits 38334 39468 +1134
+ Misses 26634 25870 -764
- Partials 3566 3629 +63
*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 exact-key delete, the quota recheck under GRAPH_SPACE_LOCK and the updateGraphIfExists guard read correctly, but the new refusal while PD still has graphs under the space turns PD graph records left by earlier failed graph drops into a permanent graph-space deletion failure. Evidence: read the full diff at 1b7976a plus GraphManager.dropGraph/clearGraphSpace/dropGraphSpace, HstoreSessionsImpl.clear on master (catch (PDException ignored)), StandardHugeGraph.clearBackend, StoreNodeService.getQuota, PartitionService.delGraph/updateGraph, PDService.delGraph, PDClient and the PD REST GraphAPI/StoreAPI mappings; checked PartitionService is a single instance per PD so its synchronized methods do serialize quota updates with delGraph; all latest-head CI checks pass, including pd_store / pd, which runs PDCoreSuiteTest with the new GraphSpaceCleanupTest.
| throw new PDException(Pdpb.ErrorType.UNKNOWN_VALUE, "Invalid graph space name"); | ||
| } | ||
| synchronized (GRAPH_SPACE_LOCK) { | ||
| if (hasGraphs(name, partitions)) { |
There was a problem hiding this comment.
Important: Graph records PD keeps for graphs the Server no longer has now block graph-space deletion for good. On master, HstoreSessionsImpl.clear() ignored a failed delGraph (catch (PDException ignored)), and GraphManager.dropGraph removes the graph config before g.clearBackend(). A graph dropped while PD was briefly unavailable therefore finished dropping on the Server but left its shop/<graph>/... records in PD. dropGraphSpace only drops graphs in this Server's this.graphs map, so it never reaches those records. This check then finds them under shop/ and refuses, and the Server reports Failed to remove graph space 'shop' statistics from PD and keeps the space. Every retry fails the same way, while the same deletion succeeded before this PR. The message does not name the graph. Neither the Server API nor PD's REST API can remove a PD graph record with no Server graph behind it: PD's only REST delete is DELETE /v1/store/{storeId}. The workaround is to find the name in GET /v1/graphs, recreate a graph with that name, and drop it.
Please handle records that no Server graph owns. For example, before calling deleteGraphSpace, have dropGraphSpace call delGraph for every PD graph under name + "/" that has no graph config left in the meta store. At minimum, put the remaining graph names in this error so an operator can act on it. A test with a leftover shop/old/g record and no matching Server graph would cover the case.
…raphspace-statistics-cleanup
Name remaining PD graph records without deleting their data. Bound diagnostic names and preserve the PD reason in Server errors. Document operator recovery and cover orphan records and payload bounds.
|
|
||
| If cleanup is refused, the Server response includes the PD reason, remaining | ||
| record count and up to ten PD graph names (each limited to 128 characters). | ||
| Inspect PD's authenticated `GET /v1/graphs` endpoint for the full identifiers and |
There was a problem hiding this comment.
Minor, non-blocking: the recommended GET /v1/graphs listing is not complete for this recovery step. At this head, GraphAPI.getGraphs() filters to names ending in /g, while ConfigService.deleteGraphSpace() rejects every record under the space prefix, including schema /m records. If such names are truncated or exceed the ten-name diagnostic limit, the recommended listing will not supply the remaining full identifiers. Please point this paragraph (and the matching exception hint) to the existing GET /v1/partitionsAndStats diagnostic endpoint: PartitionAPI.getPartitionsAndStats() retains every graph-name key, including records with no partitions. No new API or automatic deletion is needed. This is a static source-trace finding at ed61fdcc422acbfc07c52b06736dc31e7017af4a; I have not exercised historical-orphan recovery against a running cluster.
Use the existing partitionsAndStats diagnostic for all PD graph keys. Cover both data and schema records while retaining the deletion guard. Explain full identifiers and bounded error samples in the runtime guide.
…raphspace-statistics-cleanup
Explain the clearing steps already completed before an old-PD RPC failure. Retain space configuration and distinguish unsupported from other RPC failures. Share graph-name filtering and bounded diagnostic formatting.
Before → After
Before: Deleting a GraphSpace can leave PD quota and usage records behind; delayed collection can recreate stale state.
After: Delete only the exact GraphSpace record, refuse cleanup while PD still knows graphs in that space, and coordinate cleanup with quota updates. Propagate Store/PD failures instead of reporting successful deletion.
Scope and validation
Upgrade every PD node before using the new cleanup RPC. Cleanup does not restore already-deleted graphs, purge historical orphans, or make cross-replica create/delete atomic. Focused lifecycle and concurrency tests are author-reported; production-replica behavior remains unverified.
Source proposal: 🔗 hugegraph/hugegraph#336.