fix(ingestion) containers: Adding platform instance to container keys #4279
Conversation
| @@ -13,18 +13,35 @@ def test_guid_generator(): | |||
|
|
|||
| def test_guid_generator_with_empty_instance(): | |||
| key = builder.SchemaKey( | |||
| database="test", schema="Test", platform="mysql", instance=None | |||
| database="test", | |||
There was a problem hiding this comment.
There are some instance="PROD" in line 7 and 42 that may be semantically misleading. I would suggest to update them accordingly too.
There was a problem hiding this comment.
Thanks I renamed it to make it clearer a bit
Reworking container id generation
Running black
treff7es
force-pushed
the
platform_instance_container_id_fix
branch
from
74e8ad4
to
f4caaf3
Compare
treff7es
changed the title
treff7es
force-pushed
the
platform_instance_container_id_fix
branch
from
0d7f822
to
7693e81
Compare
| # Find container ids need to be migrated | ||
| container_id_map: Dict[str, str] = {} | ||
| # Get all the containers need to be migrated | ||
| containers = get_container_for_migration(env) |
There was a problem hiding this comment.
nitpick: maybe name this get_containers_for_migration
|
|
||
| entity_type = _get_type_from_urn(target_urn) | ||
| relationshipType = relationship["type"] | ||
| aspect_name = migration_utils.get_aspect_name_from_relationship_type_and_entity( |
There was a problem hiding this comment.
this method is very suspicious :)
| "containerKey", | ||
| "container", | ||
| "domains", | ||
| "containerProperties" "editableContainerProperties", |
There was a problem hiding this comment.
Should these be separated by newline?
| "mlfeature": "mlFeatureProperties", | ||
| "mlprimarykey": "mlPrimaryKeyProperties", | ||
| }, | ||
| "IsPartOf": {"container": "container", "dataset": "container"}, |
There was a problem hiding this comment.
dashboard and chart can also technically have container aspect, btw
| if entity_type.lower() == "mlprimarykey": | ||
| return "mlPrimaryKeyProperties" | ||
| aspect_map = { | ||
| "Produces": {"datajob": "dataJobInputOutput"}, |
There was a problem hiding this comment.
This seems a bit difficult to maintain.
It also does not account for cases where the same edge type can be between multiple source dest nodes ... For examples "glossaryTerm ASSOCIATED_WITH dataset" and "tag ASSOCIATED_WITH dataset"... cc @gabe-lyons - maybe a reason not to do this?
| new_urn: str, | ||
| ) -> DictWrapper: | ||
| if aspect_name == "dataJobInputOutput": | ||
| class UrnListModifier: |
There was a problem hiding this comment.
just curious - why is this better?
There was a problem hiding this comment.
Flake8 is complaining about the high complexity of the method because of the too many if branches.
| if aspect_names is not None: | ||
| for a in aspect_names: | ||
| if a in aspect_map: | ||
| aspect_value = aspect_map[a] | ||
| assert isinstance(aspect_value, DictWrapper) | ||
| new_mcp = MetadataChangeProposalWrapper( | ||
| entityUrn=dst_urn, | ||
| entityType="dataset", | ||
| entityType=entity_type, |
| ) | ||
|
|
||
|
|
||
| def get_outgoing_relationships_dataset(urn: str) -> Iterable[Dict]: |
There was a problem hiding this comment.
suffixing in _dataset is confusing, given that "dataset" also means something special to datahub :) Consider renaming this to something that won't easily be confused
| class SchemaKey(DatabaseKey): | ||
| schema: str | ||
| db_schema: str = Field(alias="schema") |
There was a problem hiding this comment.
why change to db_schema?
There was a problem hiding this comment.
schema is an internal property of Pydantic and you can't use it. :(
That's why I had to set an alias to workaround this.
| @@ -534,7 +534,7 @@ def gen_database_key(self, database: str) -> DatabaseKey: | |||
| return DatabaseKey( | |||
| database=database, | |||
There was a problem hiding this comment.
We don't support instance in glue?
| environment: Optional[str] = None | ||
|
|
||
| @root_validator(pre=True) | ||
| def check_instance_environment(cls, values: Dict[str, Any]) -> Dict[str, Any]: |
There was a problem hiding this comment.
Okay this clears things up. Never an env inside..
There was a problem hiding this comment.
This makes it a bit confusing for folks trying to understand how container keys are generated. I would naturally think the contract is that this object is turned into a dict then hashed, but really theres some logic inside.
…ontainer_id_fix # Conflicts: # metadata-ingestion/src/datahub/cli/cli_utils.py # metadata-ingestion/src/datahub/ingestion/source/sql/bigquery.py
Checklist