Re: [PR] fix(server): sync hstore schema cache clears [hugegraph]

["imbajin (via GitHub)" <gi...@apache.org>]

imbajin commented on PR #3011:
URL: https://github.com/apache/hugegraph/pull/3011#issuecomment-4377640539

   Thanks for the update. The current direction looks much better scoped than the earlier `Userdata` normalization idea.
   
   From my reading, the current PR mechanism is:
   
   - keep the local `EventHub` schema-cache listener for same-JVM cache events
   - add one JVM-wide `MetaManager` listener for cross-server schema-cache-clear events
   - publish graph-scoped schema-cache-clear events on schema add/remove
   - include a per-JVM `source` id so the publisher can skip its own watch echo
   - clear all V2 schema cache layers on remote JVMs: `schema-id`, `schema-name`, and the array attachment cache
   - continue accepting legacy plain-string event values from older publishers
   
   Could you please update the PR description/comment with a compact before -> after diagram? I think that will make the design much easier to review, especially since most of the added code is test coverage.
   
   Suggested structure:
   
   ```text
   Before
   
   +----------+       schema add/remove       +---------------+
   | Server A | ----------------------------> | HStore / Meta |
   +----------+                               +---------------+
        |
        | local V2 cache updated
        v
   +-------------------+
   | Server A cache    |
   | fresh             |
   +-------------------+
   
   +-------------------+
   | Server B cache    |
   | stale             |
   +-------------------+
   
   Problem:
   Server B had no effective cross-JVM V2 schema-cache-clear listener, so it could
   keep serving stale schema from local cache.
   ```
   
   ```text
   After
   
   +----------+       schema add/remove       +---------------+
   | Server A | ----------------------------> | HStore / Meta |
   | source A |                               +---------------+
   +----------+                                      |
        |                                            |
        | publish schema-cache-clear                 |
        | {"graph":"DEFAULT-g","source":"A"}         |
        v                                            v
   +-------------------+                    +-------------------+
   | Server A listener |                    | Server B listener |
   | source == A       |                    | source != B       |
   | skip self echo    |                    | clear V2 caches   |
   +-------------------+                    +-------------------+
                                                   |
                                                   v
                                         +---------------------+
                                         | schema-id cache     |
                                         | schema-name cache   |
                                         | array attachment    |
                                         +---------------------+
   ```
   
   A few points I would like to confirm before merge:
   
   1. `resetMetaListenerForReconnect()` does not seem to be wired to any actual `MetaManager` / `MetaDriver` reconnect callback.
   
      If there is an existing call path, could you point it out? If not, please adjust the comment so it is clear this is only a future/manual recovery hook, not a complete reconnect recovery mechanism in this PR.
   
   2. The `~create_time` String/Date round-trip should stay out of this PR, but please mention it as a follow-up.
   
      The self-echo skip avoids forcing the publisher to immediately reload its freshly created schema from backend, but remote nodes can still reload schema after cache invalidation. So the backend userdata serialization issue still exists; it is just outside the current PR scope.
   
   3. Please add a short compatibility note.
   
      Something like:
   
      > This PR does not change persisted schema data. It only changes the runtime schema-cache-clear meta-event value so new events can carry a per-JVM `source` id. New consumers still accept legacy plain-string graph names.
   
   Overall, the design now looks reasonable to me. The main remaining thing is to make the lifecycle and compatibility boundaries explicit so future readers do not assume the reconnect and `~create_time` issues are fully solved here.


-- 
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: issues-unsubscribe@hugegraph.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org