Allow custom catalogstore to trigger catalog refresh #26796
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
NingGUO520
changed the title
NingGUO520
changed the title
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
NingGUO520
force-pushed
the
refresh-catalog-in-manager
branch
from
06942f3 to
b585cde
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
NingGUO520
force-pushed
the
refresh-catalog-in-manager
branch
from
b585cde to
bed8718
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
NingGUO520
force-pushed
the
refresh-catalog-in-manager
branch
from
bed8718 to
e15b25f
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
1 similar comment
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
NingGUO520
force-pushed
the
refresh-catalog-in-manager
branch
from
d5e4a19 to
a68e7fc
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
Reviewer's GuideThis PR empowers Trino to detect external catalog configuration changes by registering a CatalogManager callback in the CatalogStore SPI and implementing a reconciliatory refreshCatalogsFromStore method in CoordinatorDynamicCatalogManager, while providing default interface methods for backward compatibility and end-to-end tests via a FilePollingCatalogStore. Class diagram for updated CatalogStore and CoordinatorDynamicCatalogManagerclassDiagram
CatalogStore <|.. CoordinatorDynamicCatalogManager
CatalogStore : +void removeCatalog(CatalogName catalogName)
CatalogStore : +void setCatalogManager(Object catalogManager)
CatalogStore : +Collection<StoredCatalog> getCatalogs()
CatalogStore : <<interface>>
CatalogStore : +interface StoredCatalog
CatalogStore : +StoredCatalog.name()
CatalogStore : +StoredCatalog.loadProperties()
CatalogStore : +StoredCatalog.version()
CoordinatorDynamicCatalogManager : +void refreshCatalogsFromStore()
CoordinatorDynamicCatalogManager : +void dropCatalog(CatalogName catalogName, boolean exists)
CoordinatorDynamicCatalogManager : +void createCatalog(...)
CoordinatorDynamicCatalogManager : -CatalogStore catalogStore
CoordinatorDynamicCatalogManager : -CatalogFactory catalogFactory
CoordinatorDynamicCatalogManager : -Map activeCatalogs
CoordinatorDynamicCatalogManager : -Map allCatalogs
CoordinatorDynamicCatalogManager : -Lock catalogsUpdateLock
CoordinatorDynamicCatalogManager : -State state
CoordinatorDynamicCatalogManager : +CoordinatorDynamicCatalogManager(CatalogStore, CatalogFactory, Executor)
CoordinatorDynamicCatalogManager --> CatalogStore
CoordinatorDynamicCatalogManager --> CatalogFactory
CoordinatorDynamicCatalogManager --> CatalogConnector
CatalogConnector --> Catalog
Catalog --> CatalogHandle
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/trino-main/src/test/java/io/trino/connector/TestFilePollingCatalogStore.java:73-74` </location>
<code_context>
+ assertThat(manager.getCatalog(new CatalogName("my_catalog")).get().getConnectorName())
+ .isEqualTo(new ConnectorName("memory"));
+
+ // Wait for initial poll - should not trigger a refresh since nothing changed
+ Thread.sleep(150);
+ assertThat(manager.getRefreshCount()).isEqualTo(0);
+
</code_context>
<issue_to_address>
**nitpick (testing):** Tests rely on sleep for timing, which may lead to flakiness.
Using Thread.sleep and polling can cause unreliable tests, particularly in CI. Please use synchronization methods or event hooks to reliably detect refresh completion.
</issue_to_address>
### Comment 2
<location> `core/trino-main/src/test/java/io/trino/connector/TestFilePollingCatalogStore.java:127` </location>
<code_context>
+ throw new AssertionError("Expected refresh count " + expectedCount + " but got " + manager.getRefreshCount());
+ }
+
+ private static class FilePollingCatalogStore
+ implements CatalogStore, Closeable
+ {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for the case where setCatalogManager is called multiple times.
Testing repeated calls to setCatalogManager will help verify that polling is managed correctly and prevent resource leaks or unintended side effects.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Wait for initial poll - should not trigger a refresh since nothing changed | ||
| Thread.sleep(150); |
There was a problem hiding this comment.
nitpick (testing): Tests rely on sleep for timing, which may lead to flakiness.
Using Thread.sleep and polling can cause unreliable tests, particularly in CI. Please use synchronization methods or event hooks to reliably detect refresh completion.
| throw new AssertionError("Expected refresh count " + expectedCount + " but got " + manager.getRefreshCount()); | ||
| } | ||
|
|
||
| private static class FilePollingCatalogStore |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for the case where setCatalogManager is called multiple times.
Testing repeated calls to setCatalogManager will help verify that polling is managed correctly and prevent resource leaks or unintended side effects.
hi, i already signed the CLA, could you check the email please ? |
1 participant
Description
This PR introduces a mechanism that allows a CatalogStore implementation to proactively trigger a refresh of the catalogs managed by the
CoordinatorDynamicCatalogManager. This enables Trino to dynamically respond to external changes in catalog configuration without requiring a restart.Motivation and Context
#26760
Part of the motivation for this work is to support dynamic credential updates for catalogs. For example, a custom CatalogStore that resolves secrets from a service like HashiCorp Vault needs a way to refresh those secrets when they are rotated.
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: