bitflicker64 commented on code in PR #3157:
URL: https://github.com/apache/hugegraph/pull/3157#discussion_r4040406053
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/SchemaDriver.java:
##########
@@ -98,24 +117,75 @@ public static void init(PDConfig pdConfig) {
}
public static void init(PDConfig pdConfig, int cacheSize, long expiration)
{
- SchemaDriver instance = INSTANCE.get();
- if (instance != null) {
- throw new NotAllowException(
- "The SchemaDriver [cacheSize=%s, expiration=%s, " +
- "client=%s] has already been initialized and is not " +
- "allowed to be initialized again", instance.caches.limit(),
- instance.caches.expiration(), instance.client);
+ synchronized (LIFECYCLE_LOCK) {
+ if (destroying) {
+ throw new NotAllowException("The SchemaDriver is being
destroyed");
+ }
+ SchemaDriver instance = INSTANCE.get();
+ if (instance != null) {
+ throw new NotAllowException(
+ "The SchemaDriver [cacheSize=%s, expiration=%s, " +
+ "client=%s] has already been initialized and is not " +
+ "allowed to be initialized again",
instance.caches.limit(),
+ instance.caches.expiration(), instance.client);
+ }
+ INSTANCE.set(new SchemaDriver(pdConfig, cacheSize, expiration));
Review Comment:
⚠️ Important. The driver is constructed while `LIFECYCLE_LOCK` is held, so
`destroy()` now blocks on PD reachability.
`new SchemaDriver(...)` runs inside this `synchronized (LIFECYCLE_LOCK)`
block, and the constructor calls `listenMetaChanges()`, which registers four
watches. Each `KvClient.listen()` reaches `AbstractClient.resetStub()` and a
blocking `getMembers()` per peer, budgeted by
`KvClient.asyncStubResetTimeoutMillis()` = `min(grpcTimeOut, 5000)` ms —
roughly 5 s per watch against a slow or unreachable PD, up to ~20 s for all
four. `destroy()` acquires the same lock at line 140 and waits that whole time
before it can even begin unpublishing.
On `master` the constructor ran outside any lock
(`INSTANCE.compareAndSet(null, new SchemaDriver(...))`) and `destroy()` took no
lock at all, so shutdown was never gated on PD reachability. This is the mirror
of the "destroy pins the monitor" problem already fixed earlier in this PR.
Requested change: reserve publication under the lock with an `initializing`
flag, construct the driver outside the lock, then re-acquire it to publish — so
no network I/O happens while the lifecycle lock is held.
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/SchemaDriver.java:
##########
@@ -98,24 +117,75 @@ public static void init(PDConfig pdConfig) {
}
public static void init(PDConfig pdConfig, int cacheSize, long expiration)
{
- SchemaDriver instance = INSTANCE.get();
- if (instance != null) {
- throw new NotAllowException(
- "The SchemaDriver [cacheSize=%s, expiration=%s, " +
- "client=%s] has already been initialized and is not " +
- "allowed to be initialized again", instance.caches.limit(),
- instance.caches.expiration(), instance.client);
+ synchronized (LIFECYCLE_LOCK) {
+ if (destroying) {
+ throw new NotAllowException("The SchemaDriver is being
destroyed");
Review Comment:
⚠️ Important. `init()` now throws for the whole duration of a `destroy()`,
and the only production caller has no handling for it.
`destroy()` unpublishes `INSTANCE` first (line 145) and keeps `destroying =
true` until `closeResources()` finishes, so this guard rejects every `init()`
made in that window. The only production caller is
`SchemaGraph.schemaDriverInit()`
(`hugegraph-struct/src/main/java/org/apache/hugegraph/SchemaGraph.java:56-65`),
which calls `init()` precisely when `getInstance()` returns `null`:
```java
if (SchemaDriver.getInstance() == null) {
synchronized (SchemaDriver.class) {
if (SchemaDriver.getInstance() == null) {
SchemaDriver.init(this.pdConfig);
}
}
}
return SchemaDriver.getInstance();
```
On `master`, `destroy()` kept `INSTANCE` published until cleanup finished,
so a `new SchemaGraph(...)` racing with a destroy saw a non-null instance,
skipped `init()` and returned normally. At this head the same race propagates
`NotAllowException("The SchemaDriver is being destroyed")` out of the
`SchemaGraph` constructor. The line-64 `return SchemaDriver.getInstance()` can
also hand back `null` when a destroy lands just after a successful `init()`,
and `loadConfig()` dereferences it immediately.
Requested change: have `init()` return the instance it published (or add a
`getOrInit`) so the caller uses that reference instead of re-reading
`INSTANCE`, and make the `destroying` window wait for completion — or be
retried by `SchemaGraph` — rather than surfacing as an exception to graph
construction.
##########
hugegraph-pd/hg-pd-client/src/main/java/org/apache/hugegraph/pd/client/AbstractClient.java:
##########
@@ -98,79 +104,154 @@ public static <T extends AbstractStub> T setAsyncParams(T
stub, PDConfig config)
new Authentication(config.getUserName(),
config.getAuthority()));
}
- protected AbstractBlockingStub getBlockingStub() throws PDException {
+ protected synchronized AbstractBlockingStub getBlockingStub() throws
PDException {
if (proxy.getBlockingStub() == null) {
- synchronized (this) {
- if (proxy.getBlockingStub() == null) {
- String host = resetStub();
- if (host.isEmpty()) {
- throw new
PDException(Pdpb.ErrorType.PD_UNREACHABLE_VALUE,
- "PD unreachable, pd.peers=" +
config.getServerHost());
- }
- }
+ String host = resetStub(stubResetTimeoutMillis());
+ if (host.isEmpty()) {
+ throw new PDException(Pdpb.ErrorType.PD_UNREACHABLE_VALUE,
+ "PD unreachable, pd.peers=" +
config.getServerHost());
}
}
return setBlockingParams(proxy.getBlockingStub(), config);
}
- protected AbstractStub getStub() throws PDException {
+ protected synchronized AbstractStub getStub() throws PDException {
if (proxy.getStub() == null) {
- synchronized (this) {
- if (proxy.getStub() == null) {
- String host = resetStub();
- if (host.isEmpty()) {
- throw new
PDException(Pdpb.ErrorType.PD_UNREACHABLE_VALUE,
- "PD unreachable, pd.peers=" +
config.getServerHost());
- }
- }
+ String host = resetStub(asyncStubResetTimeoutMillis());
+ if (host.isEmpty()) {
+ throw new PDException(Pdpb.ErrorType.PD_UNREACHABLE_VALUE,
+ "PD unreachable, pd.peers=" +
config.getServerHost());
}
}
return setAsyncParams(proxy.getStub(), config);
}
+ protected synchronized void invalidateAsyncStub() {
Review Comment:
🧹 Minor. This no-arg `invalidateAsyncStub()` has no caller.
`git grep invalidateAsyncStub` at this head matches only the declaration
here, the `Channel`-scoped overload at line 133, and the two uses of that
overload (`AbstractClient.java:351` and `KvClient.java:439`). Nothing in
`hg-pd-client`, `hg-pd-test`, `hugegraph-struct` or the server calls this form.
Requested change: drop it, or document why it is an intended extension point
— it is new `protected` surface on a client base class, so leaving it unused
invites a subclass to call the unconditional variant and clear a stub another
thread just installed.
##########
hugegraph-pd/hg-pd-client/src/main/java/org/apache/hugegraph/pd/client/AbstractClient.java:
##########
@@ -98,79 +104,154 @@ public static <T extends AbstractStub> T setAsyncParams(T
stub, PDConfig config)
new Authentication(config.getUserName(),
config.getAuthority()));
}
- protected AbstractBlockingStub getBlockingStub() throws PDException {
+ protected synchronized AbstractBlockingStub getBlockingStub() throws
PDException {
Review Comment:
⚠️ Important. Making `getBlockingStub()`/`getStub()` fully `synchronized`
removes the lock-free cached-stub path, so healthy unary calls now queue behind
a reconnect.
On `master` both methods used double-checked locking and returned without
touching the client monitor whenever the stub was already cached:
```java
protected AbstractBlockingStub getBlockingStub() throws PDException {
if (proxy.getBlockingStub() == null) {
synchronized (this) { ... resetStub(); }
}
return setBlockingParams(proxy.getBlockingStub(), config);
}
```
At this head every call acquires the monitor, and `resetStub()` runs under
that same monitor while making blocking `getMembers()` calls bounded by
`stubResetTimeoutMillis()` (= `grpcTimeOut * hostCount`, 180 s at the 60 s
default with three peers) or 5 s for `KvClient`'s async stub. So while one
watch reconnect sits in `getStub()` → `resetStub()`, every unrelated
`put`/`get`/`delete`/`scanPrefix`/`lock`/`unlock`/`keepAlive` on the same
client blocks — even though its blocking stub is still valid. `PdMetaDriver`
uses one `KvClient` for both the watches and all unary metadata operations
(`hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/meta/PdMetaDriver.java:66-209`),
so this is the server's hot metadata path.
Requested change: keep the double-checked fast path — publish the stub
through a `volatile` field or an `AtomicReference` on `AbstractClientStubProxy`
so the read is safe — and enter the monitor only when a reset is actually
needed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
