-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Glue Schema Registry schema replication converter #352
base: master
Are you sure you want to change the base?
Glue Schema Registry schema replication converter #352
Conversation
…ation-mm2-converter New cross region replication mm2 converter
…into gsr-schema-replication-converter
…rsions of the existing methods to ensure source schema compatibility with target schema
…2 versions of the existing methods to ensure source schema compatibility with target schema
… versions of the existing methods to ensure source schema compatibility with target schema
…nSchemaReplicationConverter
…ixed issue with how default configs are set with TARGET specific configs in AWSGlueCrossRegionSchemaReplicationConverter and GlueSchemaRegistryConfiguration
import software.amazon.awssdk.services.glue.model.RegisterSchemaVersionResponse; | ||
import software.amazon.awssdk.services.glue.model.RegistryId; | ||
import software.amazon.awssdk.services.glue.model.SchemaId; | ||
import software.amazon.awssdk.services.glue.model.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We avoid using *
imports in this code base. Can you please change this back?
glueSchemaRegistryConfiguration.getSourceRegion() != null && | ||
glueSchemaRegistryConfiguration.getSourceEndPoint() != null) { | ||
|
||
glueSourceClientBuilder = GlueClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating multiple clients here, can we pass in a region and create multiple instances of this client in different regions?
* @return Map of SchemaV2 with VersionIds | ||
* @throws AWSSchemaRegistryException on any error during the schema creation | ||
*/ | ||
public Map<SchemaV2, UUID> createSchemaV2(String schemaName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why SchemaV2?
|
||
import java.util.Map; | ||
import java.util.UUID; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above on imports.
*/ | ||
@AllArgsConstructor | ||
@Value | ||
public class SchemaV2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we re-use the existing Schema
class?
/** | ||
* Target Registry Name. | ||
*/ | ||
public static final String TARGET_REGISTRY_NAME = "target.registry.name"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Converter specific code must be in converter.
|
||
private void validateRequiredConfigsIfPresent(Map<String, ?> configs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to re-use any existing configs here?
import java.util.stream.Collectors; | ||
|
||
@Slf4j | ||
public class KafkaHelper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already present?
out.log
Outdated
@@ -0,0 +1 @@ | |||
nohup: ./run-local-tests.sh: No such file or directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this file?
@@ -60,6 +63,9 @@ public class GlueSchemaRegistryDeserializationFacade implements Closeable { | |||
@VisibleForTesting | |||
protected LoadingCache<UUID, Schema> cache; | |||
|
|||
@VisibleForTesting | |||
protected LoadingCache<UUID, SchemaV2> cacheV2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed if we can figure out how to re-use Schema
?
…ions to schema replication converter.
…tial MM2 schema sync
…schema replciation converter module
…schema replciation converter module
Issue #, if available:
Replicate GSR schema and schema versions from one region to another
Description of changes:
Written a new Kafka Converter to replicate schema and schema versions cross-region
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.