bitflicker64 commented on code in PR #3329:
URL: https://github.com/apache/hugegraph/pull/3329#discussion_r4265613977
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -2274,53 +2352,63 @@ public void dropGraph(String graphSpace, String name,
boolean clear) {
"Retry deletion after owners exit, or restart to
resume task admission",
graphName);
}
+ // Cleanup failure must precede metadata removal and destructive
clear.
+ this.unregisterRpcServices(g);
this.removingGraphs.add(graphName);
try {
this.metaManager.removeGraphConfig(graphSpace, name);
this.metaManager.notifyGraphRemove(graphSpace, name);
} catch (Exception e) {
+ // No backend clear has started; retain RPC for the same open
local owner.
+ synchronized (this) {
+ if (this.graphs.get(graphName) == g && !g.closed()) {
+ try {
+ this.registerRpcServices(g);
+ } catch (RuntimeException | Error restoration) {
+ if (e != restoration) {
+ e.addSuppressed(restoration);
+ }
+ }
+ }
+ }
throw new HugeException(
- "Failed to remove graph config of '%s'", name, e);
+ "Failed to remove graph config of '%s'", e, name);
}
g.clearBackend();
- try {
- g.close();
- } catch (Exception e) {
- if (!g.closed()) {
- throw new HugeException("Failed to close graph '%s',
please retry later", e, graphName);
+ }
+ try {
+ g.close();
+ } catch (RuntimeException | Error failure) {
+ throw failure;
Review Comment:
Important: this catch changes two things `dropGraph` did on master when
`g.close()` throws, and both leave a server worse off than before.
1. The exception type. `StandardHugeGraph.close()` throws a plain
`IllegalStateException` when a thread still has an open transaction or the
scheduler is not drained (`StandardHugeGraph.java:1168-1170`). Master wrapped
that in `HugeException("Failed to close graph '%s', please retry later")`. Here
it is rethrown as is. `graphRemoveHandler` only catches `HugeException` around
`this.dropGraph(parts[0], parts[1], false)` (`GraphManager.java:2773-2779`), so
the first graph that cannot close now escapes the `for` loop and the remaining
names in the same watch response are never dropped on this server.
`PdMetaDriver.extractValuesFromResponse` returns one value per event, so a
response can carry several graphs (for example after `clearGraphSpace`).
`ConsumerWrapper.accept` logs the error and nothing replays it.
2. The case where the graph did close. `close()` sets `closed = true` and
then runs the second group of steps (`unregisterRpcServices`, the cache
`closeGraph` calls, `storeProvider::close`, `LockUtil.destroy`), rethrowing the
first failure at the end. Master logged that (`if (!g.closed()) throw ...;
LOG.warn(...)`) and went on to remove the graph from `graphs` and
`HugeFactory`, recycle the quota and fire `GRAPH_DROP`. Now the method exits
here, after `removeGraphConfig` and `clearBackend()` have already run, and the
closed graph stays in `graphs`, in `HugeFactory` and in the Gremlin bindings. A
second DELETE cannot finish the job: with `clear=true` it stops at
`g.taskScheduler()` (`E.checkState(scheduler != null, ...)`, the scheduler was
removed by the successful drain) or at `clearBackend()` ->
`checkGraphNotClosed()`. On a peer (`clear=false`) nobody retries at all, and
`graphAddHandler` skips any name still present in `graphs`, so a graph created
again under that name is never
loaded on that server until restart.
Requested change: keep master's handling for a graph that reports
`closed()`: log the failure and continue into the synchronized block so the
local entry, factory binding, quota and `GRAPH_DROP` are still cleaned up. For
a graph that is not closed, keep throwing `HugeException` (or widen the catch
in `graphRemoveHandler`) so one busy graph does not stop the rest of the batch.
If the owner must be retained when RPC cleanup inside `close()` fails, the
DELETE retry needs a path that works on an already closed graph, plus a test
that retries with `clear=true`; `checkLegacyDrop` only retries the quota
failure with `clear=false`.
##########
hugegraph-commons/hugegraph-rpc/src/main/java/org/apache/hugegraph/rpc/RpcProviderConfig.java:
##########
@@ -54,27 +55,54 @@ private <T, S extends T> String addService(String graph,
providerConfig.setInterfaceId(interfaceId)
.setRef(serviceImpl);
- E.checkArgument(!this.configs.containsKey(serviceId),
+ E.checkArgument(this.configs.putIfAbsent(serviceId, providerConfig) ==
null,
"Not allowed to add service already exist: '%s'",
serviceId);
- this.configs.put(serviceId, providerConfig);
return serviceId;
}
@Override
public void removeService(String serviceId) {
- ProviderConfig<?> config = this.configs.remove(serviceId);
- E.checkArgument(config != null,
- "The service '%s' doesn't exist", serviceId);
- config.unExport();
+ boolean[] removed = {false};
+ this.configs.computeIfPresent(serviceId, (key, config) -> {
+ config.unExport();
+ removed[0] = true;
+ return null;
+ });
+ E.checkArgument(removed[0], "The service '%s' doesn't exist",
serviceId);
}
@Override
- public void removeAllService() {
- for (ProviderConfig<?> config : this.configs.values()) {
+ public boolean removeService(String serviceId, Object serviceImpl) {
+ boolean[] removed = {false};
+ this.configs.computeIfPresent(serviceId, (key, config) -> {
+ if (config.getRef() != serviceImpl) {
+ return config;
+ }
config.unExport();
+ removed[0] = true;
+ return null;
+ });
+ return removed[0];
+ }
+
+ @Override
+ public void removeAllService() {
+ for (String serviceId : this.configs.keySet()) {
+ this.configs.computeIfPresent(serviceId, (key, config) -> {
+ config.unExport();
+ return null;
+ });
+ }
+ }
+
+ void exportAll(ServerConfig server, int timeout) {
+ for (String serviceId : this.configs.keySet()) {
+ this.configs.computeIfPresent(serviceId, (key, config) -> {
+ config.setServer(server).setTimeout(timeout).export();
Review Comment:
Minor: `exportAll` now runs once for every graph published after startup
(`GraphManager.registerRpcServices` calls `this.rpcServer.exportAll()`), and
each run calls `config.setServer(server)` on every provider already in the map.
In sofa-rpc 5.7.6 `ProviderConfig.setServer(ServerConfig)` appends
(`this.server.add(server)`, `ProviderConfig.java:497-503`), and `export()` then
returns early because the provider is already exported. So each provider's
server list gains one duplicate entry per later publication, and `unExport()`
later calls `unRegisterProcessor` once per duplicate. With N late graphs that
is about N^2 list entries across the 2N notifier providers, which adds up on
servers that lazily load many graphs from PD.
Requested change: only set the server and timeout on configs that have not
been exported yet (for example skip configs whose `getServer()` is already
non-empty), or export just the two services the new graph added.
--
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]
