yuqi1129 commented on code in PR #12404:
URL: https://github.com/apache/gravitino/pull/12404#discussion_r3931795222
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -105,8 +106,9 @@ protected <R, E extends Throwable> R doWithCatalog(
NameIdentifier ident, ThrowableFunction<CatalogManager.CatalogWrapper,
R> fn, Class<E> ex)
throws E {
try {
- CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(ident);
- return fn.apply(c);
+ try (CatalogLease lease = catalogManager.acquireCatalogLease(ident)) {
+ return fn.apply(lease.wrapper());
Review Comment:
You're right, and the gap is actually wider than the dispatcher:
`EntityCombinedTable.of(alteredTable)` is returned all the way to the REST
layer, so the connector object is consumed after any dispatcher-scoped lease
would have been released.
An operation-scoped lease would narrow the window and remove the repeated
acquisitions, but it would not close it — the complete fix is a request-scoped
lease, which means threading a lease through ~50 dispatcher entry points and
the REST boundary. I'd rather not grow this PR that far, since it already
strictly improves on `main`, where there is no lease at all.
What I did do here is stop overclaiming: the `CatalogChangeLogListener` and
`EntityChangeLogPoller` javadoc now say explicitly that the lease covers the
operation but not the connector object it returns (2101c93641). I'll open a
follow-up issue for the request-scoped lease and link it here — happy to
reconsider if you'd prefer it in this PR.
--
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]
