Ingest schema metadata from Glue schema registry #8858

deepak-shingavi-cko · 2023-09-19T12:05:56Z

Datahub currently does not support Metadata ingestion from Glue Schema registry.
Datahub users can configure ingestions to ingest Kafka topic's schema definition with documentation by sample ingestion file.

source:
    type: kafka
    config:
        schema_registry_class: 'datahub.ingestion.source.kafka_schema_registry_base:GlueSchemaRegistry'
        registry_name: '<AWS Schema registry name>'
        aws_account_region: '<AWS account region>'
        connection:
            bootstrap: '${KAFKA_BOOTSTRAP_SERVER:-localhost:9092}'
            consumer_config:
                security.protocol: '${KAFKA_PROPERTIES_SECURITY_PROTOCOL:-SASL_SSL}'
                sasl.mechanism: '${KAFKA_PROPERTIES_SASL_MECHANISM:-SCRAM-SHA-512}'
                sasl.username: '${KAFKA_PROPERTIES_SASL_USERNAME}'
                sasl.password: '${KAFKA_PROPERTIES_SASL_PASSWORD}'

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

…eepak-shingavi-cko/datahub into support-glue-registry-metadata

mayurinehate

Hey @deepak-shingavi-cko, I'm planning to take a look at this PR in next 1-2 days.

Can you please fix the lint failure ? This can help on how to do that - https://datahubproject.io/docs/metadata-ingestion/developing/#code-style

Also, did you get chance to test this change in your local environment with real glue and kafka instance ? It would also be great if you can write few unit tests on similar lines as test_confluent_schema_registry.

mayurinehate

Needs some more work in addition to lint fix and writing unit tests.

Glad to help with any queries.

mayurinehate · 2023-09-27T08:31:04Z

metadata-ingestion/docs/sources/kafka/kafka.md

+By default, it follows the [Confluent's Kafka Schema registry](https://docs.confluent.io/platform/current/schema-registry/index.html)
+and supports the `JSON`, `AVRO` and `PROTOBUF` schema types.


Not sure what this line means here

mayurinehate · 2023-09-27T08:34:46Z

metadata-ingestion/setup.py

@@ -148,6 +148,7 @@ def get_long_description():
    # Deal with a version incompatibility between botocore (used by boto3) and urllib3.
    # See https://github.com/boto/botocore/pull/2563.
    "botocore!=1.23.0",
+    "aws-glue-schema-registry==1.1.2"


This should probably go against "kafka" dependencies in setup.py

Or can add aws_common as "kafka" plugin dependency given that boto is also required.

mayurinehate · 2023-09-27T08:39:31Z

metadata-ingestion/src/datahub/ingestion/source/glue_schema_registry.py

+
+class GlueSchemaRegistry(KafkaSchemaRegistryBase):
+    """
+    This is confluent schema registry specific implementation of datahub.ingestion.source.kafka import SchemaRegistry


Suggested change

This is confluent schema registry specific implementation of datahub.ingestion.source.kafka import SchemaRegistry

This is glue schema registry specific implementation of datahub.ingestion.source.kafka_schema_registry_base import KafkaSchemaRegistryBase

mayurinehate · 2023-09-28T07:59:05Z

metadata-ingestion/src/datahub/ingestion/source/glue_schema_registry.py

+        self.schema_registry_client = SchemaRegistryClient(self.glue_client, registry_name=source_config.registry_name)
+
+        self.known_schema_registry_subjects: List[str] = []
+        logger.warning(f"Get subjects from schema registry will be empty as we are using Glue.")


Suggested change

logger.warning(f"Get subjects from schema registry will be empty as we are using Glue.")

logger.warning("Get subjects from schema registry will be empty as we are using Glue.")

mayurinehate · 2023-09-28T08:12:13Z

metadata-ingestion/src/datahub/ingestion/source/kafka.py

+    )
+    aws_account_region: str = pydantic.Field(
+        default={},
+        description="The name of the Glue Schema Registry from where the schema metadata will be ingested.",


Description needs to be updated here.

Can we add a class GlueSchemaRegistryOptions and define these fields inside the class.
such that kafka config has a field:
glue_schema_registry_options: Optional[GlueSchemaRegistryOptions] # Required if using Glue Schema Registry

mayurinehate · 2023-09-28T08:12:32Z

metadata-ingestion/src/datahub/ingestion/source/glue_schema_registry.py

+        schema_type_str: str = "key" if is_key_schema else "value"
+        topic_subject: Optional[str] = 'key' if is_key_schema else "value"
+
+        print(f"topic_subject=={topic_subject is not None}")


change to debug log

mayurinehate · 2023-09-28T08:18:11Z

metadata-ingestion/src/datahub/ingestion/source/glue_schema_registry.py

+            if ref_subject in schema_seen:
+                continue
+            reference_schema: RegisteredSchema = (
+                self.schema_registry_client.get_latest_version(ref_subject)


What is the different between glue_client.get_schema_version vs schema_registry_client.get_latest_version and glue_client.get_latest_version. Can we use same method at all places ?

mayurinehate · 2023-09-28T08:35:36Z

metadata-ingestion/src/datahub/ingestion/source/glue_schema_registry.py

+    RegisteredSchema,
+    Schema,
+    SchemaReference
+


Not clear why we would need confluent_kafka imports here.
The idea to support additional schema registry is to provide implementation for following method for new schema registry

def get_schema_metadata( self, topic: str, platform_urn: str ) -> Optional[SchemaMetadata]:

Assuming that glue client and aws schema registry client has all methods, classes for interfacing with schema, these imports should not be necessary.

mayurinehate · 2023-09-28T08:39:48Z

metadata-ingestion/src/datahub/ingestion/source/glue_schema_registry.py

+                registered_schema = self.glue_client.get_schema_version(
+                    SchemaId={
+                        'SchemaName': topic + '-' + topic_subject,
+                        'RegistryName': registry_name


Suggested change

'RegistryName': registry_name

'RegistryName': self.config.registry_name

Ingest schema metadata from Glue schema registry

c19b4ac

github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Sep 19, 2023

Merge branch 'master' into support-glue-registry-metadata

9a87472

deepak-shingavi-cko marked this pull request as ready for review September 19, 2023 12:15

deepak-shingavi-cko marked this pull request as draft September 19, 2023 12:26

deepak-shingavi-cko added 5 commits September 19, 2023 13:39

move the config to KafkaSourceConfig

473ea3c

Merge branch 'support-glue-registry-metadata' of https://github.com/d…

ff2cf8c

…eepak-shingavi-cko/datahub into support-glue-registry-metadata

shifting configurable params to source config (yaml)

bce9286

update documentation

93a5d42

doc and update dependency

5b722ff

vercel bot deployed to Preview September 19, 2023 14:19 View deployment

Merge branch 'master' into support-glue-registry-metadata

040d9ef

deepak-shingavi-cko marked this pull request as ready for review September 20, 2023 08:19

vercel bot had a problem deploying to Preview September 20, 2023 08:49 Failure

Merge branch 'master' into support-glue-registry-metadata

d586922

vercel bot deployed to Preview September 25, 2023 08:59 View deployment

hsheth2 requested a review from mayurinehate September 26, 2023 03:41

mayurinehate reviewed Sep 26, 2023

View reviewed changes

mayurinehate reviewed Sep 28, 2023

View reviewed changes

hsheth2 added the community-contribution PR or Issue raised by member(s) of DataHub Community label Sep 29, 2023

hsheth2 added the pending-submitter-response Issue/request has been reviewed but requires a response from the submitter label Oct 23, 2023

shirshanka added the on-deck PR or Issue that will be reviewed and/or addressed by the DataHub Maintainers in future cycles label Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingest schema metadata from Glue schema registry #8858

Ingest schema metadata from Glue schema registry #8858

deepak-shingavi-cko commented Sep 19, 2023 •

edited

Loading

mayurinehate left a comment

mayurinehate left a comment

mayurinehate Sep 27, 2023

mayurinehate Sep 27, 2023

mayurinehate Sep 27, 2023

mayurinehate Sep 27, 2023

mayurinehate Sep 28, 2023

mayurinehate Sep 28, 2023

mayurinehate Sep 28, 2023

mayurinehate Sep 28, 2023

mayurinehate Sep 28, 2023

mayurinehate Sep 28, 2023

		By default, it follows the [Confluent's Kafka Schema registry](https://docs.confluent.io/platform/current/schema-registry/index.html)
		and supports the `JSON`, `AVRO` and `PROTOBUF` schema types.

	This is confluent schema registry specific implementation of datahub.ingestion.source.kafka import SchemaRegistry
	This is glue schema registry specific implementation of datahub.ingestion.source.kafka_schema_registry_base import KafkaSchemaRegistryBase

	logger.warning(f"Get subjects from schema registry will be empty as we are using Glue.")
	logger.warning("Get subjects from schema registry will be empty as we are using Glue.")

	'RegistryName': registry_name
	'RegistryName': self.config.registry_name

Ingest schema metadata from Glue schema registry #8858

Are you sure you want to change the base?

Ingest schema metadata from Glue schema registry #8858

Conversation

deepak-shingavi-cko commented Sep 19, 2023 • edited Loading

Checklist

mayurinehate left a comment

Choose a reason for hiding this comment

mayurinehate left a 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

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

deepak-shingavi-cko commented Sep 19, 2023 •

edited

Loading