roryqi commented on code in PR #11407:
URL: https://github.com/apache/gravitino/pull/11407#discussion_r3348067752
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -1008,6 +1008,12 @@ private boolean containsUserCreatedSchemas(
* @throws NoSuchCatalogException If the specified catalog does not exist.
*/
public CatalogWrapper loadCatalogAndWrap(NameIdentifier ident) throws
NoSuchCatalogException {
+ CatalogWrapper wrapper = catalogCache.get(ident,
this::loadCatalogInternal);
+ if (wrapper.catalog() != null) {
+ return wrapper;
+ }
+
+ catalogCache.asMap().remove(ident, wrapper);
Review Comment:
Suggestion: add a short comment explaining the double-`get` + conditional
remove.
The new logic here is correct, but the intent (why we `get` twice and why
the removal is conditional) is not obvious from the code alone. A brief comment
would help future maintainers and reviewers, e.g.:
```java
// The cached wrapper has already been closed (catalog() == null), e.g. by
cache
// invalidation/removal handling. Evict it and reload a fresh one. Use a
conditional
// remove so we do not clobber a wrapper that another thread may have just
reloaded
// into the cache concurrently.
catalogCache.asMap().remove(ident, wrapper);
```
(Optional) The Javadoc of this method could also mention the "reload on
closed cached wrapper" behavior so the contract is explicit.
--
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]
