Skip to content
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

feat(ingestion): adding superset ingestion source #2425

Merged
merged 19 commits into from
Apr 22, 2021

Conversation

gabe-lyons
Copy link
Contributor

Ingests via Superset's REST API. Currently populates chart/dashboard information. Followup work

  • integrate with Superset's log system to enable push-based events into datahub
  • ingest metrics, filters and dimensions into the model

image

image

image

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)

@@ -7,5 +8,8 @@ export function getLogoFromPlatform(platform: string) {
if (platform.toLowerCase() === 'looker') {
return lookerLogo;
}
if (platform.toLowerCase() === 'superset') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've got to push this from backend. Forgot about this.

Copy link
Collaborator

@hsheth2 hsheth2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - left a couple minor comments

test_response = self.session.get(f"{self.config.connect_uri}/api/v1/database")
if test_response.status_code == 200:
pass
# TODO(Gabe): how should we message about this error?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just do test_response.raise_for_status() and let the caller deal with the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sg- I can handle that in a follow up.

f"urn:li:dataset:("
f"{platform_urn},{database_name + '.' if database_name else ''}"
f"{schema_name + '.' if schema_name else ''}"
f"{table_name},{self.env})"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely not necessary for this PR, but we have mce_builders.py which lets you do builder.make_dataset_urn(platform, name, env) - my eventual goal is to put all the URN construction stuff there

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shirshanka shirshanka merged commit c7b49de into datahub-project:master Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants