Dynamic catalogs improvements #26918
Open
+179
−47
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntroduce explicit catalog load statuses by replacing the boolean loaded flag with a CatalogStatus enum and exposing LOADED/FAILED_TO_LOAD in system.metadata.catalogs, updating related metadata operations and tests accordingly. Entity relationship diagram for updated Catalog and CatalogInfo data typeserDiagram
Catalog {
CatalogName catalogName
CatalogHandle catalogHandle
ConnectorName connectorName
ConnectorServices catalogConnector
ConnectorServices informationSchemaConnector
ConnectorServices systemConnector
CatalogStatus catalogStatus
}
CatalogInfo {
string catalogName
CatalogHandle catalogHandle
ConnectorName connectorName
CatalogStatus catalogStatus
}
CatalogStatus {
enum: LOADED, FAILED_TO_LOAD
}
CatalogStatus ||--o| Catalog : "catalogStatus"
CatalogStatus ||--o| CatalogInfo : "catalogStatus"
Catalog ||--o| CatalogInfo : "catalogHandle"
Catalog ||--o| CatalogInfo : "connectorName"
Class diagram for Catalog, CatalogInfo, and CatalogStatus changesclassDiagram
class Catalog {
-CatalogName catalogName
-CatalogHandle catalogHandle
-ConnectorName connectorName
-ConnectorServices catalogConnector
-ConnectorServices informationSchemaConnector
-ConnectorServices systemConnector
-CatalogStatus catalogStatus
+CatalogStatus getCatalogStatus()
+boolean isFailed()
+static Catalog failedCatalog(...)
}
class CatalogInfo {
-String catalogName
-CatalogHandle catalogHandle
-ConnectorName connectorName
-CatalogStatus catalogStatus
}
class CatalogStatus {
<<enum>>
LOADED
FAILED_TO_LOAD
}
Catalog --> CatalogStatus
CatalogInfo --> CatalogStatus
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
homar
force-pushed
the
homar/fix_dynamic_catalogs
branch
2 times, most recently
from
307c31a to
74678d0
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Add explicit ORDER BY clauses in system.metadata.catalogs queries in tests to ensure deterministic row ordering and avoid flakiness.
- Update the Javadoc for Metadata.listCatalogs to clearly state that it returns all catalogs along with their load status (LOADED or FAILED_TO_LOAD).
- Consider aligning the naming of the "state" column with the CatalogStatus enum (e.g., rename to "status") for consistency across code and metadata schemas.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add explicit ORDER BY clauses in system.metadata.catalogs queries in tests to ensure deterministic row ordering and avoid flakiness.
- Update the Javadoc for Metadata.listCatalogs to clearly state that it returns all catalogs along with their load status (LOADED or FAILED_TO_LOAD).
- Consider aligning the naming of the "state" column with the CatalogStatus enum (e.g., rename to "status") for consistency across code and metadata schemas.
## Individual Comments
### Comment 1
<location> `core/trino-main/src/main/java/io/trino/metadata/Metadata.java:482` </location>
<code_context>
Optional<CatalogHandle> getCatalogHandle(Session session, String catalogName);
/**
- * Lists all defined catalogs (both loaded properly and failed ones).
+ * Gets all the loaded catalogs
*/
List<CatalogInfo> listCatalogs(Session session);
</code_context>
<issue_to_address>
**suggestion:** The new comment may be misleading about the method's behavior.
The new comment does not match the method's actual behavior, which returns all catalogs, including those that failed. Please update the comment to accurately describe the method's output.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
homar
force-pushed
the
homar/fix_dynamic_catalogs
branch
from
74678d0 to
acc1cb0
Compare
findepi
approved these changes
Oct 13, 2025
| informationSchemaConnector, | ||
| systemConnector, | ||
| true); | ||
| systemConnector); |
There was a problem hiding this comment.
Revert "metadata.catalogs should only list loaded catalogs"
This reverts commit a7d9d91.
Please include rationale for the revert
Comment on lines
45
to
46
| Session session = testSessionBuilder().build(); | ||
| QueryRunner queryRunner = DistributedQueryRunner.builder(session) |
There was a problem hiding this comment.
Suggested change
| Session session = testSessionBuilder().build(); | |
| QueryRunner queryRunner = DistributedQueryRunner.builder(session) | |
| QueryRunner queryRunner = DistributedQueryRunner.builder(testSession()) |
| } | ||
|
|
||
| @Test | ||
| public void testHealthyCatalog() |
| } | ||
|
|
||
| @Test | ||
| public void testUnhealthyCatalog() |
There was a problem hiding this comment.
Suggested change
| public void testUnhealthyCatalog() | |
| public void testNewUnhealthyCatalog() |
| connectorServicesProvider.loadInitialCatalogs(); | ||
|
|
||
| assertQuery("SHOW CATALOGS", "VALUES 'healthy_catalog', '" + catalogName + "', 'system'"); | ||
| assertQueryFails("CREATE TABLE %s.default.test_table (age INT)".formatted(catalogName), ".*Catalog '%s' failed to initialize and is disabled.*".formatted(catalogName)); |
There was a problem hiding this comment.
assert what's the error for a SELECT statement over such catalog
Comment on lines
18
to
19
| LOADED, | ||
| FAILED_TO_LOAD, |
There was a problem hiding this comment.
"loaded catalogs" inuititively means those loaded into the system, aka. defined
maybe we can call these options: OPERATIONAL / FAILING
There was a problem hiding this comment.
Sure I can try. cc @kokosing
| } | ||
|
|
||
| @Test | ||
| public void testCatalogTableShowsCorrectStatusWhenCatalogIsNotLoadedCorrectly() |
There was a problem hiding this comment.
testCatalogNotLoadedCorrectly ?
| } | ||
|
|
||
| @Test | ||
| public void testCatalogTableShowsCorrectStatusWhenCatalogsAreLoadedCorrectly() |
There was a problem hiding this comment.
testNewCatalog / testNewCatalogStatus?
| Optional<CatalogMetadata> catalogMetadata = Optional.empty(); | ||
| if (catalogManager.getCatalog(catalog).isPresent()) { | ||
| Optional<Catalog> optionalCatalog = catalogManager.getCatalog(catalog); | ||
| if (optionalCatalog.isPresent() && optionalCatalog.get().getCatalogStatus() == LOADED) { |
There was a problem hiding this comment.
What we do here:
- we first read from
catalogManager
1.1. we conditionally re-read something ingetCatalogMetadataForWrite - we then issue unconditional drop in
catalogManager - depending on the status if the (1) read,
systemSecurityMetadata.catalogDroppedis called
What we want to guarantee is that:
if a catalog ceased to exist, systemSecurityMetadata.catalogDropped has been called
Which may not be the case when e.g.
- (pre-existing) catalog did not exist and was created between (1) and (2)
- (new) catalog did exist but unhealthy and was fixed between (1) and (2)
It's pre-existing problem. Let's file a follow-up issue and link TODO comment here.
| assertQueryFails("CREATE CATALOG %s USING memory WITH (\"memory.max-data-per-node\" = '128MB')".formatted(catalogName), ".*Catalog '%s' already exists.*".formatted(catalogName)); | ||
|
|
||
| catalogManager.dropCatalog(new CatalogName(catalogName), false); | ||
| assertUpdate("DROP CATALOG " + catalogName); |
There was a problem hiding this comment.
this assert the DROP did not fail
assert also it's indeed gone.
homar
force-pushed
the
homar/fix_dynamic_catalogs
branch
from
acc1cb0 to
3b5769e
Compare
homar
force-pushed
the
homar/fix_dynamic_catalogs
branch
2 times, most recently
from
d759251 to
b6f1b79
Compare
homar
force-pushed
the
homar/fix_dynamic_catalogs
branch
from
b6f1b79 to
5da1431
Compare
This reverts commit a7d9d91. Not showing those catalogs that failed to load basically hides them from users as this information would be available only in coordinator logs. This is not user friendly.
homar
force-pushed
the
homar/fix_dynamic_catalogs
branch
from
5da1431 to
f76079b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
There are several things in this PR:
metadata.catalogsreturn all catalogs (included those that didn't load correctly), together with the their statusAdditional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:
Summary by Sourcery
Track and surface the load state of catalogs by replacing the boolean loaded flag with a CatalogStatus enum, exposing a new state column in system.metadata.catalogs, and adding end-to-end tests for healthy and failed dynamic catalogs.
New Features:
Enhancements:
Tests: