Add ownership to Ingestion Source resolvers #11902
Conversation
github-actions
bot
added
product
| """ | ||
| The urn of the ingestion source | ||
| """ | ||
| ingestionSource: String |
There was a problem hiding this comment.
Since this is an URN and not the actual resolved ingestionSource entity object, I'd recommend naming this ingestionSourceUrn for clarity and consistency
| @@ -47,15 +47,16 @@ public CompletableFuture<IngestionSource> get(final DataFetchingEnvironment envi | |||
| context.getOperationContext(), | |||
| Constants.INGESTION_SOURCE_ENTITY_NAME, | |||
| new HashSet<>(ImmutableSet.of(urn)), | |||
| ImmutableSet.of(Constants.INGESTION_INFO_ASPECT_NAME)); | |||
| ImmutableSet.of( | |||
There was a problem hiding this comment.
Should we also fetch the ownership aspect?
There was a problem hiding this comment.
I think this might be a mistake, fetching ORIGIN instead of OWNERSHIP
| @@ -101,7 +101,8 @@ public CompletableFuture<ListIngestionSourcesResult> get( | |||
| new HashSet<>(entitiesUrnList), | |||
| ImmutableSet.of( | |||
| Constants.INGESTION_INFO_ASPECT_NAME, | |||
| Constants.INGESTION_SOURCE_KEY_ASPECT_NAME)); | |||
| Constants.INGESTION_SOURCE_KEY_ASPECT_NAME, | |||
| Constants.OWNERSHIP_ASPECT_NAME)); | |||
| @@ -101,7 +101,8 @@ public CompletableFuture<ListIngestionSourcesResult> get( | |||
| new HashSet<>(entitiesUrnList), | |||
There was a problem hiding this comment.
Is there any unit test we need to update for this class?
| @@ -47,15 +47,16 @@ public CompletableFuture<IngestionSource> get(final DataFetchingEnvironment envi | |||
| context.getOperationContext(), | |||
| Constants.INGESTION_SOURCE_ENTITY_NAME, | |||
There was a problem hiding this comment.
Is there any unit test we need to update for this class?
| @@ -8,6 +9,7 @@ | |||
| import com.linkedin.datahub.graphql.generated.IngestionSource; | |||
| import com.linkedin.datahub.graphql.generated.StringMapEntry; | |||
| import com.linkedin.datahub.graphql.generated.StructuredReport; | |||
| import com.linkedin.datahub.graphql.types.common.mappers.OwnershipMapper; | |||
There was a problem hiding this comment.
Is there any unit test we need to update for this class?
| """ | ||
| Ownership metadata of the IngestionSource | ||
| """ | ||
| ownership: Ownership |
There was a problem hiding this comment.
How does ExectionRequest have Ownership? I'm not seeing this field populated by your GraphQL java changes (only ingestion source)
| @@ -414,6 +434,11 @@ type IngestionSource { | |||
| """ | |||
| platform: DataPlatform | |||
|
|
|||
| """ | |||
There was a problem hiding this comment.
This makes sense - the only changes in here do not
There was a problem hiding this comment.
Neither dataHubExecutionRequest NOR dataHubIngestionSource entity types have the ownership aspect: https://github.com/datahub-project/datahub/blob/master/metadata-models/src/main/resources/entity-registry.yml#L289
So these changes won't exactly work without it.
Also, there is currently no way to set the ownership of an ingestion source through the UI or otherwise, so this PR is generally NOT applicable for Open Source DataHub.
This one requires further discussion.
Cheers
John
hsheth2
added
pending-submitter-response
Add "ownership" aspect with the entity "DatahubExecutionRequest" so that we could filter jobs by the owner's corp id
and will be able to send notification to the owners of the DatahubExecutionRequest
Checklist