fix(hstore): invalidate graph registration after clear - #3141
Conversation
Signed-off-by: ulofiai <monsterking@tutamail.com>
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The production change clears the stale registration marker after graph deletion, but the added regression test only exercises the private helper and does not validate the public clear/recreate lifecycle. Evidence: Static review of HstoreSessionsImpl.clear() and HstoreSessionsImplTest.testClearInitializedGraphStateRepeatedly(); no CI checks reported for this exact head.
| try { | ||
| for (int i = 0; i < 50; i++) { | ||
| Assert.assertTrue(initializedGraphs.add(graphName)); | ||
| clearInitializedGraph.invoke(null, graphName); |
There was a problem hiding this comment.
clearInitializedGraph() via reflection instead of exercising HstoreSessionsImpl.clear(). It would still pass if the finally call at HstoreSessionsImpl.java:188 were removed or misplaced, and it does not verify the reported clear/recreate lifecycle. Please cover the public clear path (ideally the repeated clear/recreate sequence) and assert that a subsequent open re-registers the graph, or use controllable fakes to verify that behavior.
There was a problem hiding this comment.
Thanks for catching this. You're right—the current test only exercises the private helper and would not fail if clear() stopped invoking it. I'll replace it with a public open → clear → open lifecycle test using controllable fakes and verify that the graph is re-registered after clear.
Signed-off-by: ulofiai <monsterking@tutamail.com>
|
Updated in e053609. The regression test now exercises the public open() -> clear() -> open() lifecycle for 50 iterations using controllable store and PD fakes, and verifies that the graph is re-registered after every clear. Removing the marker invalidation from clear() now makes the test fail. |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The marker is cleared, but the actual HstoreStore clear/reopen path can still skip graph re-registration. Evidence: exact-head HstoreStore.open() at lines 216-223, BackendSessionPool.useSession() at lines 73-81, and the passing HstoreSessionsImpl-only regression test; latest-head CI checks are all successful.
| } catch (PDException ignored) { | ||
|
|
||
| } finally { | ||
| clearInitializedGraph(this.graphName); |
There was a problem hiding this comment.
HstoreStore.open() returns through sessions.useSession() when its session pool is still open, while BackendSessionPool.useSession() only reattaches/detects the existing session and never calls HstoreSessionsImpl.open() or setGraph(). Thus clear(true) followed by store.open(config) can delete the PD graph and then skip recreating it; the new test calls HstoreSessionsImpl.open() directly and does not cover this path. Please make the store-level reopen re-register the graph and add a HstoreStore clear → open regression test.
Fixes the HStore clear/recreate lifecycle defect described in #3090.
This focused change invalidates the local
infoInitializedGraphmarker after a successful store-side graph deletion. The marker is cleared in afinallyblock, so a lateropen()can re-register the same graph even when the PD deletion call reports an error.A regression test repeats the initialization-state add/clear transition 50 times to guard against stale JVM lifecycle state.
Validation:
git diff --checkMaven tests were not run because Maven is unavailable in the execution image.