Copilot commented on code in PR #13698:
URL: https://github.com/apache/gravitino/pull/13698#discussion_r4217171365
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -454,9 +460,14 @@ public GravitinoMetalake retrieveMetalake(String
metalakeName) {
private void loadCatalogs(GravitinoMetalake metalake) {
String metalakeName = metalake.name();
+ // Null when the detailed listing failed, in which case each catalog is
loaded on its own.
+ Map<String, Catalog> catalogInfos = listCatalogInfos(metalake);
String[] allCatalogNames;
try {
- allCatalogNames = metalake.listCatalogs();
+ allCatalogNames =
+ catalogInfos != null
+ ? catalogInfos.keySet().toArray(new String[0])
+ : metalake.listCatalogs();
Review Comment:
`listCatalogInfos()` uses `null` as a sentinel for “fall back to per-catalog
load”, which increases the chance of future mistakes (e.g., new call sites
forgetting the null-check or dereferencing). Consider returning an
`Optional<Map<String, Catalog>>` (or a small result type like `CatalogListing {
Map<String,Catalog> infos; boolean usedFallback; }`) so the fallback decision
is explicit and enforced by the type system.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorManager.java:
##########
@@ -633,6 +647,46 @@ metalakeName, catalogName, trinoCatalogName, provider,
toErrorMessage(e)),
}
}
+ /**
+ * Lists the catalogs of a metalake together with their details in a single
request, so that a
+ * refresh cycle costs the server one call (and one audit record) instead of
one per catalog.
+ *
+ * @return the catalogs keyed by name in listing order, or null if the
listing failed and the
+ * caller should load the catalogs one by one
+ */
+ @Nullable
+ private Map<String, Catalog> listCatalogInfos(GravitinoMetalake metalake) {
+ Catalog[] catalogs;
+ try {
+ catalogs = metalake.listCatalogsInfo();
+ } catch (Exception e) {
+ // The detailed listing fails as a whole when any single catalog cannot
be resolved by the
+ // server or converted by the client, e.g. a catalog type this client
does not know. Loading
+ // the catalogs one by one keeps such a failure confined to the catalog
that caused it.
+ if (perCatalogLoadMetalakes.add(metalake.name())) {
+ LOG.warn(
+ e,
+ "Failed to list catalog details of metalake %s, loading its
catalogs one by one "
+ + "until the listing recovers. Every refresh then sends one
request per catalog.",
+ metalake.name());
+ } else {
+ LOG.debug(
+ e,
+ "Failed to list catalog details of metalake %s, loading catalogs
one by one",
+ metalake.name());
+ }
+ return null;
+ }
+ if (perCatalogLoadMetalakes.remove(metalake.name())) {
+ LOG.info("Catalog details listing of metalake %s recovered.",
metalake.name());
+ }
+ Map<String, Catalog> catalogInfos = new LinkedHashMap<>();
+ for (Catalog catalog : catalogs) {
+ catalogInfos.put(catalog.name(), catalog);
+ }
+ return catalogInfos;
Review Comment:
The `LinkedHashMap` is created with the default capacity, which may resize
for larger catalog lists. Since `catalogs.length` is known, consider
initializing with an appropriate capacity (e.g., derived from
`catalogs.length`) to avoid rehash/resizes during refresh.
--
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]
