Migrate Data Catalog to Dataplex Knowledge Catalog for CDC templates#3927
Migrate Data Catalog to Dataplex Knowledge Catalog for CDC templates#3927stankiewicz wants to merge 25 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical failure in the CDC pipeline caused by the deprecation of the legacy Google Cloud Data Catalog API. By migrating the schema publishing and retrieval logic to the Dataplex Knowledge Catalog API, the pipeline maintains compatibility with current Google Cloud infrastructure standards. The changes ensure that metadata, including system types and labels, is correctly preserved and mapped to the new Dataplex entry structures. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the CDC parent and connector modules from Google Cloud Data Catalog to Google Cloud Dataplex Catalog, updating dependencies, client initializations, and schema mapping utilities to use Dataplex's aspect-based schema model. The review feedback identifies several critical issues, including resource leaks from unclosed CatalogServiceClient instances, an N+1 query performance bottleneck when retrieving entry schemas, improper handling of InterruptedException, and potential NullPointerException risks when parsing protobuf Struct fields without defensive checks.
There was a problem hiding this comment.
Code Review
This pull request migrates the CDC common library from Google Cloud Data Catalog to Google Cloud Dataplex Catalog, updating dependencies, clients, and schema mapping utilities to use Dataplex aspects. Feedback on these changes suggests propagating exceptions during asynchronous entry group creation to avoid invalid states, filtering search results in lookupPubSubEntry to verify they belong to the @pubsub entry group, and normalizing data types to lowercase in SchemaUtils to ensure robust, case-insensitive type mapping.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3927 +/- ##
============================================
- Coverage 55.59% 55.58% -0.01%
+ Complexity 7052 6731 -321
============================================
Files 1103 1116 +13
Lines 67681 68378 +697
Branches 7603 7707 +104
============================================
+ Hits 37625 38008 +383
- Misses 27636 27900 +264
- Partials 2420 2470 +50
🚀 New features to boost your workflow:
|
|
Fixes: #3921 |
There was a problem hiding this comment.
Code Review
This pull request migrates the CDC application from Google Cloud Data Catalog to Google Cloud Dataplex Catalog, updating dependencies, client implementations, and schema conversion utilities to use Dataplex aspects. The review feedback highlights several critical issues: a resource leak due to an unclosed CatalogServiceClient in DataCatalogSchemaManager, swallowed exceptions in getSchemasForEntryGroup and createEntryGroup that hinder debugging, a potential thread hang from calling .get() without a timeout, misleading error handling during JSON parsing, and a lack of string trimming when parsing comma-separated topics or subscriptions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the CDC schema management from Google Cloud Data Catalog to Google Cloud Dataplex, updating dependencies, client implementations, and schema conversion utilities to work with Dataplex Entry and EntryGroup APIs. Key feedback points include addressing a potential resource leak in PubsubUtils where SubscriptionAdminClient is repeatedly instantiated, optimizing getSchemasForEntryGroup by setting EntryView.ALL on the list request to avoid O(N) network roundtrips, and resolving a lossy integer type mapping in SchemaUtils that could cause runtime ClassCastExceptions.
| private static void setupSubscriptionClient() throws IOException { | ||
| subscriptionAdminClient = SubscriptionAdminClient.create(); | ||
| } |
There was a problem hiding this comment.
Every time getPubSubTopicFromSubscription is called, setupSubscriptionClient() unconditionally instantiates a new SubscriptionAdminClient and overwrites the static variable subscriptionAdminClient without closing the previous one. This leaks the underlying gRPC channels and threads. Only instantiate the client if it is currently null.
private static void setupSubscriptionClient() throws IOException {
if (subscriptionAdminClient == null) {
subscriptionAdminClient = SubscriptionAdminClient.create();
}
}| com.google.cloud.dataplex.v1.ListEntriesRequest request = | ||
| com.google.cloud.dataplex.v1.ListEntriesRequest.newBuilder() | ||
| .setParent(formattedParent) | ||
| .build(); | ||
| CatalogServiceClient.ListEntriesPagedResponse response = client.listEntries(request); | ||
| for (Entry entry : response.iterateAll()) { | ||
| entry = | ||
| client.getEntry( | ||
| GetEntryRequest.newBuilder() | ||
| .setName(entry.getName()) | ||
| .setView(EntryView.ALL) | ||
| .build()); |
There was a problem hiding this comment.
In getSchemasForEntryGroup, calling client.getEntry in a loop for every entry results in setView(EntryView.ALL) on the ListEntriesRequest, the returned entries will already contain all aspects, completely eliminating the need for individual getEntry calls.
| com.google.cloud.dataplex.v1.ListEntriesRequest request = | |
| com.google.cloud.dataplex.v1.ListEntriesRequest.newBuilder() | |
| .setParent(formattedParent) | |
| .build(); | |
| CatalogServiceClient.ListEntriesPagedResponse response = client.listEntries(request); | |
| for (Entry entry : response.iterateAll()) { | |
| entry = | |
| client.getEntry( | |
| GetEntryRequest.newBuilder() | |
| .setName(entry.getName()) | |
| .setView(EntryView.ALL) | |
| .build()); | |
| com.google.cloud.dataplex.v1.ListEntriesRequest request = | |
| com.google.cloud.dataplex.v1.ListEntriesRequest.newBuilder() | |
| .setParent(formattedParent) | |
| .setView(EntryView.ALL) | |
| .build(); | |
| CatalogServiceClient.ListEntriesPagedResponse response = client.listEntries(request); | |
| for (Entry entry : response.iterateAll()) { |
There was a problem hiding this comment.
invalid suggestion
|
@damccorm , IT tests are passing, which means that Debezium is able to persist schema and Dataflow template is able to read the schema. it would be great to add to release notes some details about data catalog deprecation when new release is scheduled. |
| List<String> subscriptionList; | ||
|
|
||
| List<TopicSubscriptionSchema> readSourceSchemas = | ||
| buildTopicSubscriptionSchemas( |
There was a problem hiding this comment.
Is there a reason we're moving this out of PubSubUtils in this PR? It seems like we're mixing functional and non-functional changes, which makes it harder to review/understand what is happening, and I'd prefer to avoid that if possible.
There was a problem hiding this comment.
Ack, will move what is below to pubsubutils.
|
|
||
| DebeziumToPubSubDataSender dataSender; | ||
| try { | ||
| TimeUnit.SECONDS.sleep(30); |
There was a problem hiding this comment.
Why are we introducing a sleep here?
There was a problem hiding this comment.
It takes some time for dataplex to ingest data after pubsub topic create. Will remove it as final version is based on custom entrygroup, not pubsub resource.
| pipelineOperator() | ||
| .waitForConditionsAndFinish( | ||
| createConfig(info), | ||
| createConfig(info, java.time.Duration.ofMinutes(5)), |
There was a problem hiding this comment.
Is there a reason for the short timeout?
There was a problem hiding this comment.
45 minutes didn't make sense and is bad experience in troubleshooting if you need to wait to test completion to see the logs.
| "A problem occurred when communicating with Cloud Data Catalog"); | ||
| "A problem occurred when communicating with Knowledge Catalog"); | ||
| } else { | ||
| LOG.info("New entry {}", result); |
|
|
||
| LookupEntryRequest request = | ||
| LookupEntryRequest.newBuilder().setLinkedResource(linkedResource).build(); | ||
| public abstract static class DataCatalogSchemaManager implements AutoCloseable { |
There was a problem hiding this comment.
Should we still be naming things DataCatalog<thing>?
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** Set of utilities to convert Beam Schemas to Data Catalog schemas. */ | ||
| /** Set of utilities to convert Beam Schemas to Dataplex schema aspects. */ |
There was a problem hiding this comment.
Dataplex should be Knowledge Catalog (here and elsewhere)
stankiewicz
left a comment
stankiewicz
left a comment
There was a problem hiding this comment.
Will apply changes after 18th of July
3 participants
This PR resolves the INVALID_ARGUMENT write operation failure (Project is not allowed to perform write operations due to Data Catalog deprecation) caused by the deprecation of the legacy Google Cloud Data Catalog API.
It migrates the Debezium-to-PubSub CDC pipeline's schema publishing and schema retrieval logic to use the new Dataplex Knowledge Catalog API (com.google.cloud.dataplex.v1.CatalogServiceClient).
Key Changes