[590] Add RunCatalogSync utility for synchronizing tables across catalogs #591

vinishjail97 · 2024-12-06T07:52:47Z

Important Read

Please ensure the GitHub issue is mentioned at the beginning of the PR

What is the purpose of the pull request

Introduced RunCatalogSync utility which does the following on a high level. This unblocks the ability to sync tables from a source catalog to multiple target catalogs, you can look at the sample configuration here xtable-utilities/src/test/resources/catalogConfig.yaml

Start off with SourceCatalog.
User configures multiple TargetCatalog's where tables will be synced.
XTable generates the table format metadata in storage first.
Syncs the table format metadata to TargetCatalog.

Brief change log

(for example:)

Add interface for CatalogSyncClient and CatalogSync
Add. ability to synchronize tables between source and target catalogs

Verify this pull request

(Please pick either of the following options)

This change added tests and can be verified as follows:

(example:)

org.apache.xtable.spi.sync.TestCatalogSync
org.apache.xtable.spi.sync.TestCatalogUtils

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSyncClient.java

xtable-api/src/main/java/org/apache/xtable/model/exception/CatalogRefreshException.java

xtable-api/src/main/java/org/apache/xtable/model/sync/SyncResult.java

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogUtils.java

xtable-api/src/main/java/org/apache/xtable/model/catalog/CatalogType.java

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSync.java

xtable-api/src/main/java/org/apache/xtable/conversion/ExternalCatalog.java

vinishjail97 · 2024-12-10T02:32:22Z

I'm pushing another PR for the client side changes for CatalogSync.
https://github.com/apache/incubator-xtable/pull/597/files

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSync.java

xtable-api/src/main/java/org/apache/xtable/conversion/ExternalCatalog.java

vinishjail97 · 2024-12-10T20:17:04Z

After putting more thought into it, I think we can keep the cross catalog sync as a separate function and not integrate it with TableFormatSync.

Start off with SourceCatalog. (This can be StorageCatalog as well).
User can choose multiple TargetCatalog and in each catalog there's an option to sync multiple table formats.
XTable generates the table format metadata in storage first.
Syncs the table format metadata to TargetCatalog.

This will be separate utility option in RunSync to configure a catalogConfig.yaml file.

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSync.java

xtable-api/src/main/java/org/apache/xtable/conversion/ExternalTable.java

xtable-api/src/main/java/org/apache/xtable/conversion/TargetCatalog.java

ashvina · 2024-12-12T22:37:56Z

xtable-api/src/main/java/org/apache/xtable/model/catalog/CatalogTableIdentifier.java

+/** This class represents the unique identifier for a table in a catalog. */
+@Value
+@Builder
+public class CatalogTableIdentifier {


What do you think of calling this class TableIdentifier.

Also, we should consider making the naming more generic, since it should represent all types of table identifiers. For instance, while databaseName is a popular namespace, there's also schema. In some scenarios, databaseName is synonymous with catalogName. The current two-part naming based on table and databaseName seems a bit restrictive to me.

Would you mind sharing the use cases this naming caters to?

1 is okay, included the prefix Catalog because TableIdentifier is overloaded in the dependent projects (iceberg, hudi, delta etc.) and didn't want to add another identifier with the same name.

For 2, agreed that each catalog or system has a different term for what's called a "logical grouping of tables".
I have looked up this name in different systems and have found databaseName, namespace, schema are the popular ones, a more generic name that comes to my mind is tableCollection or tableGroup ? If we can't find a generic name, choosing databaseName maybe okay ? Regardless of the name we choose the conversion interfaces for each catalog are responsible for translating the catalog table definition to this representation.

xtable-api/src/main/java/org/apache/xtable/model/catalog/CatalogTableIdentifier.java

xtable-core/src/main/java/org/apache/xtable/conversion/ConversionController.java

xtable-api/src/main/java/org/apache/xtable/spi/extractor/ConversionSource.java

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSync.java

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSyncClient.java

the-other-tim-brown · 2024-12-17T20:43:31Z

xtable-utilities/src/test/java/org/apache/xtable/utilities/TestRunCatalogSync.java

+    assertDoesNotThrow(() -> RunCatalogSync.main(args));
+  }
+
+  public static class TestCatalogImpl implements CatalogConversionSource, CatalogSyncClient {


Would it be simpler to use a mock object here?

CatalogConversionFactory contains methods which use reflection to load the classImpl provided by the user, mocking this won't be a good assertion from UT's perspective.

I saw a similar class in another test, should we just have one test implementation we reuse for these?

the-other-tim-brown · 2024-12-17T20:44:42Z

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSyncClient.java

+  String getCatalogImpl();
+
+  /** Returns the storage location of the table synced to the catalog. */
+  String getStorageDescriptorLocation(TABLE table);


What composes a storage descriptor? is it just a path?

Yes, it's the storage path for the table in the catalog.

Should we just call it getStorageLocation then?

xtable-core/src/main/java/org/apache/xtable/conversion/ConversionController.java

xtable-api/src/test/java/org/apache/xtable/spi/sync/TestCatalogSync.java

xtable-core/src/main/java/org/apache/xtable/conversion/ConversionController.java

ashvina · 2024-12-18T04:55:55Z

Hi @vinishjail97 , this PR seems to cover two distinct features: refactoring to add CatalogSyncClient and enabling syncing to multiple catalogs. The combination of these features might be contributing to the PR's size. What do you think about splitting the PR along these feature lines?

ashvina · 2024-12-18T04:58:40Z

xtable-api/src/main/java/org/apache/xtable/conversion/ConversionConfig.java

@@ -34,14 +35,21 @@ public class ConversionConfig {
  @NonNull SourceTable sourceTable;
  // One or more targets to sync the table metadata to
  List<TargetTable> targetTables;
+  // Each target table can be synced to multiple target catalogs, this is map from
+  // targetTableIdentifier to target catalogs.
+  Map<String, List<TargetCatalogConfig>> targetCatalogs;


I am curious about the expected behavior if XTable fails to update a subset of the TargetCatalogs and if that impacts the way incremental sync works?
This may have been covered elsewhere in the PR that I might have missed.

It doesn't change the behavior of existing incremental sync, the failure is returned back as part of this, btw existing XTable users can sync use incremental sync without configuring source/target catalogs, the existing RunSync class in utilities or sync function in ConversionController is not changing.

// The sync status for each catalog. List<CatalogSyncStatus> catalogSyncStatusList;

#591 (comment)

ashvina · 2024-12-18T05:30:18Z

xtable-api/src/main/java/org/apache/xtable/conversion/TargetTable.java

@@ -44,4 +44,8 @@ public TargetTable(
    this.metadataRetention =
        metadataRetention == null ? Duration.of(7, ChronoUnit.DAYS) : metadataRetention;
  }
+
+  public String getId() {


What is the contract of Id? Could you please add javadoc for public methods of the core classes ?

ashvina · 2024-12-18T05:36:15Z

xtable-api/src/main/java/org/apache/xtable/model/catalog/CatalogTableIdentifier.java

+public class CatalogTableIdentifier {
+  /**
+   * Catalogs have the ability to group tables logically, databaseName is the identifier for such
+   * logical classification. The alternate names for this field include namespace, schemaName etc.


I agree that the nesting vary depending on the database. However, the usage of a 3-part naming is quite common. The convention includes name of database, schema, and table or view. I am wondering if it would be feasible to keep the table Id as a string instead of an object whose structure can vary a lot.

ashvina · 2024-12-18T05:49:18Z

Hi @vinishjail97, This change significantly impacts XTable usage. The current description doesn't provide enough context for me. Could we set up a call or create a document to discuss this change in detail?

xtable-api/src/main/java/org/apache/xtable/conversion/TargetCatalogConfig.java

vinishjail97 · 2024-12-18T06:37:48Z

@ashvina I will create two separate PR's to avoid the confusion.

Interfaces for CatalogSyncClient and CatalogSync.
Integration of catalog syncs in XTable conversion controller using RunCatalogSync.

We are not changing the way incremental sync works for table formats, the sync method in ConversionController still exists, the functionality of syncTableAcrossCatalogs is to synchronize a table in source catalog to target catalog. If there's a need for tableFormat sync, we synchronize the table format first otherwise skip it. After that, the targetTable's metadata is synchronized to the target catalogs using the catalogTableIdentifier provided. I will add a small RFC document as well in the second PR for clarity.

…logs

vinishjail97 · 2024-12-18T10:20:14Z

@ashvina I have split this change two PR's for clarity.

I will push the PR for an RFC doc for this tomorrow but have replied to most of your comments. Regarding the class CatalogTableIdentifier let's discuss on the RFC, if we have a compatibility matrix between CatalogTableIdentifier and the naming conventions for major catalogs documented users can easily understand.

vinishjail97 · 2024-12-18T23:02:11Z

xtable-core/src/test/java/org/apache/xtable/delta/ITDeltaDeleteVectorConvert.java

@@ -1,178 +0,0 @@
-/*


Revert this change.

vinishjail97 mentioned this pull request Dec 6, 2024

Sync table formats metadata to multiple catalogs. #590

Open

2 tasks

the-other-tim-brown reviewed Dec 6, 2024

View reviewed changes

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSyncClient.java Outdated Show resolved Hide resolved

vinishjail97 marked this pull request as draft December 6, 2024 22:41

the-other-tim-brown reviewed Dec 7, 2024

View reviewed changes

vinishjail97 force-pushed the 590-CatalogSync branch from 7f307d4 to 06501c1 Compare December 9, 2024 23:29

vinishjail97 changed the title ~~[590] Add interface for CatalogSyncClient and CatalogSyncOperations~~ [590] Add interface for CatalogSyncClient and CatalogSync Dec 10, 2024

vinishjail97 marked this pull request as ready for review December 10, 2024 02:16

the-other-tim-brown reviewed Dec 10, 2024

View reviewed changes

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSync.java Outdated Show resolved Hide resolved

xtable-api/src/main/java/org/apache/xtable/conversion/ExternalCatalog.java Outdated Show resolved Hide resolved

vinishjail97 commented Dec 12, 2024

View reviewed changes

xtable-api/src/main/java/org/apache/xtable/spi/sync/CatalogSync.java Outdated Show resolved Hide resolved

ashvina reviewed Dec 12, 2024

View reviewed changes

vinishjail97 commented Dec 17, 2024

View reviewed changes

xtable-core/src/main/java/org/apache/xtable/conversion/ConversionController.java Show resolved Hide resolved

the-other-tim-brown reviewed Dec 17, 2024

View reviewed changes

ashvina reviewed Dec 18, 2024

View reviewed changes

xtable-api/src/main/java/org/apache/xtable/conversion/TargetCatalogConfig.java Show resolved Hide resolved

[590] Add RunCatalogSync utility for synchronizing tables across cata…

1c07c07

…logs

vinishjail97 force-pushed the 590-CatalogSync branch from 7ab5659 to 1c07c07 Compare December 18, 2024 10:05

vinishjail97 changed the base branch from main to 590-CatalogSync-API December 18, 2024 10:07

vinishjail97 changed the title ~~[590] Add interface for CatalogSyncClient and CatalogSync~~ [590] Add RunCatalogSync utility for synchronizing tables across catalogs Dec 18, 2024

vinishjail97 commented Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[590] Add RunCatalogSync utility for synchronizing tables across catalogs #591

[590] Add RunCatalogSync utility for synchronizing tables across catalogs #591

vinishjail97 commented Dec 6, 2024 •

edited

Loading

vinishjail97 commented Dec 10, 2024 •

edited

Loading

vinishjail97 commented Dec 10, 2024 •

edited

Loading

ashvina Dec 12, 2024

vinishjail97 Dec 13, 2024

the-other-tim-brown Dec 17, 2024

vinishjail97 Dec 17, 2024

the-other-tim-brown Dec 18, 2024

the-other-tim-brown Dec 17, 2024

vinishjail97 Dec 17, 2024

the-other-tim-brown Dec 18, 2024

ashvina commented Dec 18, 2024

ashvina Dec 18, 2024

vinishjail97 Dec 18, 2024

vinishjail97 Dec 18, 2024

ashvina Dec 18, 2024

ashvina Dec 18, 2024

ashvina commented Dec 18, 2024

vinishjail97 commented Dec 18, 2024 •

edited

Loading

vinishjail97 commented Dec 18, 2024

vinishjail97 Dec 18, 2024

[590] Add RunCatalogSync utility for synchronizing tables across catalogs #591

Are you sure you want to change the base?

[590] Add RunCatalogSync utility for synchronizing tables across catalogs #591

Conversation

vinishjail97 commented Dec 6, 2024 • edited Loading

Important Read

What is the purpose of the pull request

Brief change log

Verify this pull request

vinishjail97 commented Dec 10, 2024 • edited Loading

vinishjail97 commented Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashvina commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashvina commented Dec 18, 2024

vinishjail97 commented Dec 18, 2024 • edited Loading

vinishjail97 commented Dec 18, 2024

Choose a reason for hiding this comment

vinishjail97 commented Dec 6, 2024 •

edited

Loading

vinishjail97 commented Dec 10, 2024 •

edited

Loading

vinishjail97 commented Dec 10, 2024 •

edited

Loading

vinishjail97 commented Dec 18, 2024 •

edited

Loading