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(ingest): clean up DataHubRestEmitter return type #9286
Conversation
Also makes the Airflow hook support arbitrary args.
b22401d
to
516bded
Compare
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.
LGTM but there's some test failures:
FAILED tests/unit/test_airflow.py::test_datahub_rest_hook - datahub.configuration.common.OperationalError: ('Unable to emit metadata to DataHub GMS', {'message': "HTTPConnectionPool(host='test_host', port=8080): Max retries exceeded with url: //entities?action=ingest (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f44e1180730>: Failed to establish a new connection: [Errno -3] Temporary failure in name resolution'))"})
FAILED tests/unit/test_airflow.py::test_datahub_rest_hook_with_timeout - TypeError: __init__() got an unexpected keyword argument 'timeout_sec'
metadata-ingestion-modules/airflow-plugin/src/datahub_airflow_plugin/hooks/datahub.py
Outdated
Show resolved
Hide resolved
metadata-ingestion-modules/airflow-plugin/src/datahub_airflow_plugin/hooks/datahub.py
Outdated
Show resolved
Hide resolved
MetadataChangeProposalWrapper, | ||
], | ||
) -> Tuple[datetime, datetime]: | ||
start_time = datetime.now() |
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.
Do we want to ensure timezone utc here?
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.
I'm keeping this the same for backwards compat for now, but eventually we should just update everything to always use non-naive datetimes
…plugin/hooks/datahub.py Co-authored-by: Andrew Sikowitz <andrew.sikowitz@acryl.io>
…plugin/hooks/datahub.py Co-authored-by: Andrew Sikowitz <andrew.sikowitz@acryl.io>
…b into rest-emitter-return
Also makes the Airflow hook support arbitrary args.
Checklist