Skip to content

Conversation

Neer393
Copy link
Contributor

@Neer393 Neer393 commented Aug 28, 2025

What changes were proposed in this pull request?

This PR added support for pattern based catalog retrieval. The change is that a new method was added called getCatalogs(String catalogPattern) . If all catalogs are to be fetched, set catalogPattern to null, else if catalogPattern is set to empty string, no catalogs are returned. Also the existing getCatalogs() under the hood calls getCatalogs(null).

Why are the changes needed?

These changes are needed for IcebergHouseKeeperService where the service is expiring snapshots according to the dbpattern and tablepattern but there was no support for catalogpattern yet. This is being added in this PR

Does this PR introduce any user-facing change?

No user facing change. Only updated getCatalogs to fetch catalogs based on pattern

How was this patch tested?

This patch was tested locally by executing tests to ensure no breaking changes are introduced.

@@ -2619,7 +2619,7 @@ service ThriftHiveMetastore extends fb303.FacebookService
void create_catalog(1: CreateCatalogRequest catalog) throws (1:AlreadyExistsException o1, 2:InvalidObjectException o2, 3: MetaException o3)
void alter_catalog(1: AlterCatalogRequest rqst) throws (1:NoSuchObjectException o1, 2:InvalidOperationException o2, 3:MetaException o3)
GetCatalogResponse get_catalog(1: GetCatalogRequest catName) throws (1:NoSuchObjectException o1, 2:MetaException o2)
GetCatalogsResponse get_catalogs() throws (1:MetaException o1)
GetCatalogsResponse get_catalogs(1: GetCatalogRequest pattern) throws (1:MetaException o1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a breaking change. is the original get_catalogs() api still available for old clients?

Copy link
Contributor Author

@Neer393 Neer393 Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Should I keep the existing get_catalogs() ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if there are clients in old version call get_catalogs(), the server in new version could not response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry I have made changes in all clients like IMetaStoreClient, BaseMetaStoreClient, MetaStoreClientWrapper and ThriftHiveMetastoreClient and this get_catalogs() under the hood calls get_catalogs(null) by passing catalogPattern as null. So it's non-breaking change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I mean if the server use new api get_catalogs(pattern), but some engines that do not upgrade the client version may still call get_catalog() api, in this case the call would fail.

@@ -131,6 +131,15 @@ void alterCatalog(String catalogName, Catalog newCatalog)
*/
List<String> getCatalogs() throws MetaException, TException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can deprecate it after introducing getCatalogs(String catlogPattern)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can deprecate this but we can use it to fetch all catalogs without providing a parameter for pattern. It's like in IMetaStoreClient where we have 2 methods for getDatabases

List<String> getDatabases(String databasePattern) throws MetaException, TException;
List<String> getDatabases(String catName, String databasePattern)
      throws MetaException, TException;

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants