LuciferYang commented on code in PR #13166:
URL: https://github.com/apache/gravitino/pull/13166#discussion_r4068990241
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/catalog/GravitinoCatalogManager.java:
##########
@@ -259,4 +264,27 @@ private static void closeClient(GravitinoIdentity
identity, GravitinoClient clie
LOG.warn("Failed to close the Gravitino client of {}.", identity, e);
}
}
+
+ /**
+ * Holds a cached client so that the shutdown drain and the removal listener
can both try to close
+ * it while the underlying client is closed at most once.
+ */
+ private static class CachedClient {
+ private final GravitinoClient client;
+ private final AtomicBoolean closed = new AtomicBoolean(false);
+
+ private CachedClient(GravitinoClient client) {
+ this.client = client;
+ }
+
+ private GravitinoClient client() {
+ return client;
+ }
+
+ private void close() {
+ if (closed.compareAndSet(false, true)) {
+ client.close();
+ }
+ }
+ }
}
Review Comment:
This ordering is intentional. Setting the CAS before `client.close()` is
what guarantees at-most-once; moving it after a successful close would let the
drain and the listener both enter `client.close()` concurrently, which is the
real double close. The window you describe is not a double close or a leak: the
client is still being closed by whichever path won the CAS, so at worst
`close()` returns just before an async listener finishes that same close. That
is the documented async-removal-listener behavior (see the comment on the
`removalListener`), and it predates this change. Serializing `close()` would
add locking to the shutdown path without preventing any lost or duplicated
close, so I left it as is.
--
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]
