Repository navigation
fix(server): keep schema caches current across HStore Servers - #3477
bitflicker64 wants to merge 3 commits into
Conversation
With several Servers on HStore, a schema change made through one Server reached the others only for adds. Updates published no clear event, removes published one only with task.sync_deletion=false, and an event sent while a Server's PD watch was reconnecting was lost. A Server that kept an old copy then wrote it back over newer schema. Complete the existing clear event instead of adding a new mechanism: - publish after every committed add, update, remove and store clear or truncate, and once when a graph is opened - KvClient.listen() accepts a callback that runs each time a watch goes live; the Server clears its V2 schema caches when it runs - a publish that fails after the schema write no longer fails the call; the graph is kept in a pending set and published again with backoff and when the watch is live again - a generation counter and a lock on the shared cache keep a read, a listing or a write-through that started before a clear from caching its value after the clear - every event clears at once, events are not merged Not covered here: a write in flight on another Server while an index is created can still miss the index, a writer that stops for good between the schema write and its publish, two Servers changing the same element at once, serving cached schema while cut off from PD, and a watch stream that dies without an error. They are documented in docs/server-runtime.md.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3477 +/- ##
============================================
+ Coverage 58.53% 58.62% +0.08%
- Complexity 2717 3130 +413
============================================
Files 779 780 +1
Lines 69439 69617 +178
Branches 9577 9603 +26
============================================
+ Hits 40647 40810 +163
- Misses 25101 25114 +13
- Partials 3691 3693 +2
*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:
|
A second pass over the schema cache clear path found four defects. - A schema write is two or three PD puts. One that failed after the first put left the change partly in PD, published nothing and kept the writer's own copy. It now drops that copy, publishes, and still reports the error. The publish also runs if caching the written element fails. - The generation was advanced before the caches were changed. A read running at that moment could note the new generation, take the old value from the id cache and copy it into the array cache afterwards, where it stayed until the next clear. The generation is now advanced as the last step of every change. - A watch start during a running retry scheduled a second retry chain, which doubled the publishes and the backoff. Only the newest retry reschedules now. - After a refused write-through the "whole list is cached" flag of that schema type was never set again, so each listing read PD until the next clear. Tests: one per fix, plus tests for the publish order, the callback passed by PdMetaDriver, the guard across transactions of one graph and local invalidation events. The listing-lock test now holds a real listing, and the concurrency test checks every read while it runs. A V2 graph opened without PD no longer leaves a retry behind that fires in a later test.
Follow-up to review comments on this change. - MultiClusterSchemaCacheTest: the readers that run while an index is created were submitted and never checked, so a reader that stopped on an error left the test passing with fewer reads. Their futures are read once the index is created. - docs/server-runtime.md: a PD watch that ends with a gRPC status the PD client does not retry is not started again, so that Server keeps its cached schema until it is restarted. This is how master behaves for every meta listener and is tracked in apache#3478. - A TODO in the test's waitFor notes that the shared RestClient sets no timeouts.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
8 open issues
Responseobjects are not closed in the high-frequency reader loop (and also not closed in helper… · NewResponseobjects are not closed in the high-frequency reader loop (and also not closed in helper… · NewCachedSchemaTransactionV2Test.resetPendingClearEvents()from another test class… · NewCachedSchemaTransactionV2Test.resetPendingClearEvents()from another test class… · NewScannerisn’t closed here. Even though the underlyingInputStreamis closed by the… · NewPendingClearEventskeeps a daemonScheduledExecutorServicealive indefinitely once created,… · NewPendingClearEventskeeps a daemonScheduledExecutorServicealive indefinitely once created,… · New
What changed in this PR
This PR improves cross-Server schema cache correctness for HStore deployments by ensuring schema-cache-clear events are published on all schema write paths, retried on publish failure, and by clearing local V2 schema caches whenever the PD watch becomes live again (e.g., after disconnect/leader change), guarded by a per-graph generation to prevent stale refills.
Changes:
- Add reliable schema cache invalidation + generation guard + failed-publish retry logic to
CachedSchemaTransactionV2. - Extend PD watch plumbing (
KvClient,MetaDriver/MetaManager/GraphMetaManager,PdMetaDriver) with a “watch started” callback. - Add extensive unit + multi-cluster tests and update operational docs for the new semantics/limits.
| File | Description |
|---|---|
| hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedSchemaTransactionV2.java | Publish clear events after writes (even partial failures), retry failed publishes, clear caches on watch start, add generation-based cache guard |
| hugegraph-pd/hg-pd-client/src/main/java/org/apache/hugegraph/pd/client/KvClient.java | Add listen(..., startedCallback) and invoke callback on first live frame per watch lifecycle |
| hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/meta/MetaDriver.java | Add default listen(key, consumer, onStarted) API with fallback behavior |
| hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/meta/MetaManager.java | Plumb new schema-cache-clear listen overload with onStarted |
| hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/meta/managers/GraphMetaManager.java | Add listenSchemaCacheClear(..., onStarted) overload |
| hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/meta/PdMetaDriver.java | Implement new listen(..., onStarted) using KvClient.listen overload |
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/cache/CachedSchemaTransactionV2Test.java | New in-memory “PD store” tests for publish ordering, retries, watch start clears, generation guard, and concurrency cases |
| hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/client/KvClientTest.java | Add coverage for watch-start callback behavior across leader changes, stream errors, replaced watches |
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/cache/CachedSchemaTransactionTest.java | Adjust existing tests for watch-start callback registration and extra clear publish on graph open/store clear |
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java | Include the new V2 cache unit test class |
| hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/MultiClusterTest/MultiClusterSuiteTest.java | Include the new multi-cluster schema cache suite |
| hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/MultiClusterTest/MultiClusterSchemaCacheTest.java | New multi-cluster integration tests validating cross-Server schema cache propagation and PD restart window |
| hugegraph-cluster-test/hugegraph-clustertest-minicluster/src/main/java/org/apache/hugegraph/ct/node/AbstractNodeWrapper.java | Add restart support and improve “started” detection after restart by skipping old log bytes |
| hugegraph-cluster-test/hugegraph-clustertest-minicluster/src/main/java/org/apache/hugegraph/ct/env/AbstractEnv.java | Add PD node restart/await helpers for cluster tests |
| helm/hugegraph/docs/operations.md | Update index-status behavior description post-fix and link to runtime limits |
| docs/server-runtime.md | Document new guarantees/limits for schema caches across HStore Servers |
Review effort: Lite
| reads.add(readers.submit(() -> { | ||
| while (!stop.get()) { | ||
| Response r = client.target().path(VERTICES) | ||
| .queryParam("label", vl) | ||
| .queryParam("limit", 3).request().get(); | ||
| r.readEntity(String.class); | ||
| } | ||
| })); |
There was a problem hiding this comment.
Not changing this. readEntity(String.class) reads the whole entity and closes the entity stream, which is what releases the connection. That is the JAX-RS contract of readEntity for every type other than an input stream, so nothing stays open after each pass of the loop.
| response.readEntity(String.class); | ||
| return response.getStatus(); | ||
| } | ||
|
|
||
| private static void ok(Response response) { | ||
| String content = response.readEntity(String.class); | ||
| Assert.assertEquals("Response with status " + response.getStatus() + | ||
| " and content " + content, 2, response.getStatus() / 100); | ||
| } | ||
|
|
||
| private static JsonNode json(Response response) throws Exception { | ||
| String content = response.readEntity(String.class); | ||
| Assert.assertEquals("Response with status " + response.getStatus() + | ||
| " and content " + content, 2, response.getStatus() / 100); | ||
| return MAPPER.readTree(content); |
There was a problem hiding this comment.
Same as the reader loop above: status, ok and json each call readEntity(String.class), which closes the entity stream and releases the connection. No change.
| this.client.listen(key, (Consumer<WatchResponse>) consumer, | ||
| throwable -> { }, onStarted); |
There was a problem hiding this comment.
The stop is already visible in the log: KvClient.stopWatch logs Watch for key {} stopped after a non-retryable error at ERROR, with the status, before it calls this handler. Retryable errors never reach the handler, they reconnect, so a line here would repeat that one.
What the Server should do once a watch has stopped for good is a real gap. It is the same on master for every meta listener and is tracked in #3478. 7df011a lists it under the limits in docs/server-runtime.md.
| // A V2 graph opened without a PD leaves a clear event to send again | ||
| CachedSchemaTransactionV2Test.resetPendingClearEvents(); | ||
| this.graph = HugeFactory.open(FakeObjects.newConfig()); |
There was a problem hiding this comment.
Valid as a style point, left as it is for now. Both classes test CachedSchemaTransactionV2 and sit in the same package, the helper resets static state of that one class, and a rename is caught at compile time. A utility class for one ten-line method would add a file without changing what the tests do. If a third test class needs the reset, that is the moment to move it out.
| this.graph.close(); | ||
| this.graph = null; | ||
| } | ||
| CachedSchemaTransactionV2Test.resetPendingClearEvents(); |
There was a problem hiding this comment.
Same point as the comment on line 82 of this file, answered there: left as it is until a third test class needs the helper.
| Scanner sc = new Scanner(log, StandardCharsets.UTF_8.name()); | ||
| while (sc.hasNextLine()) { | ||
| String line = sc.nextLine(); | ||
| if (line.contains(startLine)) return true; |
There was a problem hiding this comment.
Not changing this. The Scanner only wraps log, and log is the resource of the enclosing try, so the file handle is closed on every path, the early return true included. What the Scanner holds besides that is a heap buffer that goes away with the local variable. Listing it as a resource too would create it before log.skipNBytes(...), which is correct only because Scanner reads lazily.
| if (this.executor == null) { | ||
| this.executor = Executors.newSingleThreadScheduledExecutor(r -> { | ||
| Thread thread = new Thread(r, "schema-cache-clear-retry"); | ||
| thread.setDaemon(true); | ||
| return thread; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Valid, left as it is. The executor is created only after a first failed publish and is one daemon thread that sits idle once the pending set is empty, so it holds nothing and does not block shutdown. Shutting it down and creating it again would add a lifecycle to the retry path, which already has to handle a retry that runs while a watch start schedules a new one. The classes in Server core that need a scheduler each own one the same way. It would be worth changing if Server gets a shared scheduler for jobs like this.
| if (this.pending.isEmpty()) { | ||
| this.retryDelay = RETRY_MIN_DELAY; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same point as the comment on the executor field above, answered there: one idle daemon thread after a first failed publish, left as it is.
Purpose of the PR
With several Servers on HStore, a schema change made through one Server does not reliably reach the others, and a Server that keeps an old copy can write it back over newer schema. #3235 has the failures measured on master
8828d8238.This PR completes the existing schema-cache-clear event. It does not add a new sync mechanism. There is no change to PD raft, the PD store or
kv.proto; the only PD code touched is the client library (KvClient).It is a new change written from master. It is not built on #3237 (polling, closed) and contains nothing from #3376, which stays separate follow-up work.
Scope at a glance
The right column is not solved here and this PR does not claim it. Each item is explained under "What this PR does not implement".
Main Changes
What this PR fixes
All changes are in
CachedSchemaTransactionV2unless noted.updateSchema()published nothing, so a property append,user_data, the index list of a base label and every status change stayed on the writer.removeSchema()and store clear or truncate published only withtask.sync_deletion=false. Now every committed add, update, remove and store clear or truncate publishes a clear event after its write. Every event clears at once; events are not delayed or merged.A schema write is two or three PD puts. If it fails after the first one, PD holds part of the change. That case publishes too, and the writer drops its own copy of the element; the caller still gets the error.
KvClientsubscribes again one second after a disconnect without telling the listener.KvClient.listen()has a new overload with a callback that runs each time a watch goes live (first frame of a new stream, including after a leader change or a stream error). The Server clears all its V2 schema caches when it runs, so nothing missed while the watch was down survives.Plumbing for the callback:
MetaDriver.listen(key, consumer, onStarted)as a default method that falls back to the plain listen, overridden inPdMetaDriver, and passed throughMetaManagerandGraphMetaManager. The etcd meta driver uses the fallback and gets no watch-start clear.Changed behavior of schema calls
Once the schema write has committed, a failed event publication no longer fails the schema call. The call returns success and the publication is retried in the background (item 3).
Before this PR the add path threw after the commit when the publish failed, so the caller saw an error for a change that was already stored. Inside an index rebuild job the same throw would fail the job after its status write had committed. A publish failure is logged at WARN with the graph name.
What a caller can now observe: the call succeeds while other Servers still hold the old copy until the retry goes through or their watch restarts.
How this relates to the review of #3237
That review asked for a notification design in place of polling and listed points to settle first. This is where each one stands here. Nothing from #3237 itself is revived.
Cost of an event
One event makes every other Server drop the schema cache of that graph. Its next read by name then loads the whole schema again (four prefix scans in PD).
Measured on 1 PD + 1 Store + 3 Servers on one host (12 CPUs), fresh cluster with 300 property keys, 2026-11-13: 200 updates in a row through S0 while S1 and S2 each serve schema reads by name on two threads. Two runs per build.
8828d8238So for as long as schema changes arrive back to back, schema reads on the other Servers were about 11 times slower in this setup, and they recovered when the burst ended. The cost grows with the schema: the same burst on a graph with about 950 property keys gave 180 to 190 reads per second. Ten index creations in a row cost about the same on both builds (140 to 149 ms per creation and 2,450 to 2,580 reads per second on the other Servers). I did not measure the effect on graph reads and writes, only on schema reads. Lowering this cost means sending which element changed, or merging events, and is proposed as follow-up.
What this PR does not implement
These are known limits. They are listed in
docs/server-runtime.mdas limits, and none of them is implied to be solved by this PR. Follow-up issues so far: the existing #3376, and #3478 for a watch that stops for good.GraphTransaction.prepareCommit()takes the index list from the cached label, andIndexLabelRebuildJobscans the label once and starts as soon as the task is scheduled.UNAUTHENTICATED,PERMISSION_DENIED,UNIMPLEMENTEDand six more) is removed and not started again. The watch-start clear then never runs, and that Server keeps its cached schema until it is restarted; its log has one ERROR line for the stopped watch. PD does not answer a watch with these codes itself. They can come from a proxy between Server and PD: gRPC maps HTTP 401, 403 and 404 to three of them.Two more points:
Found while reviewing this change and left out because they are independent of it:
IntObjectMap.set()grows its array by one doubling only, so the first write of a schema id above twice the current size throws. It is on master and is hit by the array cache of both schema transactions. It can no longer skip a publish, because the publish runs in afinally.SchemaDriver) without a watch-start callback. I did not examine what their cache is used for.Review scope
This PR is meant to stay at its current size and shape. For a review finding I will do the following:
The second commit is the result of such a pass over the first one. It fixes four things, each with a test: no event after a schema write that failed partway, a retry that could schedule itself twice, a generation that advanced before the caches had changed (a concurrent read could copy the old value into the array cache), and a listing flag that never recovered after a refused write-through.
The third commit follows the review comments on the mirror, hugegraph#374. It changes no production code. The index test now fails when one of its background readers stops on an error (their results were never read, so the test could pass with fewer reads than it claims), and the stopped-watch limit above is added to
docs/server-runtime.mdwith #3478 as its follow-up. The remaining comments were checked against the code and not taken: theResponsein the reader loop is released byreadEntity, theScannerreads a stream that itstrycloses, a stopped watch is already logged at ERROR by the PD client, a nullonStartedis already rejected by name in the PD client, and the shared test helper and the missing timeouts on the cluster-test HTTP client are optional (the timeouts are a TODO in the test).A second round of comments on the third commit led to no code change. The points it repeats are answered above. Two were new: the reset helper of the cache tests cannot stop a retry that is already running (listed under "Known gaps in the evidence"), and the retry thread stays after the pending set is empty. That thread is one idle daemon thread, created only after a first failed publish, and each class in Server core that needs a scheduler owns one in the same way.
Size
16 files, +2,692 / -121 (
git diff --shortstat 8828d8238 7df011acb).CachedSchemaTransactionV2,KvClient, meta plumbing)Verifying these changes
CachedSchemaTransactionV2Test(new, 35 tests): publish on every write path and after the write, failed and partial writes, failed publish and retry, watch start, and the generation guard on every cache insertion and write-through pathKvClientTest(7 new tests): the watch-start callback on first start, leader change, stream error and a replaced watchMultiClusterSchemaCacheTest(new, 5 tests): 3 PD, 3 Store and 3 ServersReproductions before and after
Same scripts for both columns, through the REST API, 1 PD + 1 Store + 3 Servers (S0, S1, S2) as local processes on one host (12 CPUs, Java 17), 2026-11-13. "Before" is master
8828d8238, "after" is the head of this branch.user_datathrough S0p1, then S1 appendsp2to the same label[a, p2]; a vertex write that setsp1returns 400 on S2[a, p1, p2]; the vertex is read back withp1through each ServerCREATINGin 30 of 30task.sync_deletion=trueIn the "after" run each Server logged the watch-start clear four times: once at start and once per PD restart. No Server logged a failed publish.
All of this ran on one host with sub-millisecond latency. It shows that the cases happen and stop happening, not how often they occur in production.
Test suites
Java 17, 2026-11-13. Format and multi cluster ran on the tree of the head commit. The other suites ran on the second commit; the third changes one cluster test and one doc file and adds no file. Server unit and HStore core ran before one doc paragraph and one comment in the cluster-test harness were added to the second commit.
mvn editorconfig:check -pl hugegraph-pd/hg-pd-test,hugegraph-server/hugegraph-test,hugegraph-cluster-test/hugegraph-clustertest-test -am -ntpmvn apache-rat:check -ntpmvn test -pl hugegraph-server/hugegraph-test -am -P unit-test -ntpmvn test -pl hugegraph-server/hugegraph-test -am -P core-test,hstore -ntpmvn test -pl hugegraph-cluster-test/hugegraph-clustertest-test -am -P multi-cluster-test -ntpMultiClusterSchemaCacheTest); the runtime log check passes. Run again on the third commit with the same resultmvn test -pl hugegraph-pd/hg-pd-test -am -P pd-client-test -ntpKvClientTest: 48), run on the first commit; the PD client is unchanged sinceThe two cache test classes were also run 12 times in a row in fresh JVMs: 12 of 12 passed.
One existing test changed:
CachedSchemaTransactionTest.testV2WarmRequestsRetainSchemaWithoutMetadataReadsAndReceiveInvalidationsexpected one PD put and now expects one at graph open and one more for the store clear.Do the tests detect the bugs
MultiClusterSchemaCacheTeston unpatched master8828d8238: 4 of 5 fail (update, append, index creation, PD restart).CachedSchemaTransactionV2Testwas run against it:testEveryPublishComesAfterItsStoreWrite,testAddPublishestestWriteThatFailsHalfwayPublishesAndDropsTheElement,testRemoveThatFailsHalfwayPublishesAndDropsTheElement,testFailedWriteKeepsItsEventWhenThePublishFailsTootestWatchStartDuringARetryLeavesOneRetrytestReadDuringALocalWriteDoesNotPutTheOldValueBack(3 of 3 runs)testRefusedWriteThroughEvictsWhatWasCachedMeanwhile,testReadsNeverGoBackBehindAWriteUnderConcurrentClearstestAClearWaitsForAListingReadFromTheCache,testListingIsNeverPartialWhileClearsRun,testReadsNeverGoBackBehindAWriteUnderConcurrentClearstestRefusedWriteThroughEvictsWhatWasCachedMeanwhileKnown gaps in the evidence
testRemoveReachesEveryServerWithSyncDeletionpasses on unpatched master, because a new graph publishes events while its system schema is set up. The unit test and the manual check in the table above cover that gate.testChangeDuringPDRestartReachesEveryServerdoes not prove that a watch was down when the change committed. It failed on unpatched master in the one control run. The evidence for the watch-start clear is the unit tests plus the Server log lines of the manual run.testReadsNeverGoBackBehindAWriteUnderConcurrentClearsandtestListingIsNeverPartialWhileClearsRundepend on timing. The first one failed in about half of the runs before the generation was advanced last, and in none of 12 after.finallyafter a failure inside the cache update has no test.Does this PR potentially affect the following parts?
KvClient.listen()overload with a started callback and a default methodMetaDriver.listen(key, consumer, onStarted). Each Server publishes one clear event per graph when it opens the graph.Documentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.Documentation files in this PR or paired hugegraph-doc PR:
docs/server-runtime.md: new section "Schema caches across HStore Servers" with the guarantees and the limitshelm/hugegraph/docs/operations.md: the paragraph on index status across Pods