-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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): migrate Cassandra source to new SDK #12695
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
❌ Your patch status has failed because the patch coverage (85.86%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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
UpstreamClass, | ||
UpstreamLineageClass, | ||
ViewPropertiesClass, | ||
) | ||
from datahub.sdk._entity import Entity |
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.
In which cases should we allow/restrict the usage of these private modules?
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 probably going to make Entity a public interface in the future - will do that in a follow-up
In general, we should avoid usages of private modules
@@ -468,6 +469,7 @@ def __init__( | |||
env=env, | |||
) | |||
super().__init__(urn) | |||
self._set_extra_aspects(extra_aspects) |
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.
given we are calling _set_extra_aspects
for both container and dataset, should we move that responsibility to the parent: super().__init__(urn, extra_aspects)
?
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.
tbh I'm not super happy with the interface setup right now
Specifically, I'm not sure if set_extra_aspects should be called immediately after the super.init, or at the end of the Dataset/Container init method
Once I have a stronger opinion on it, I'll refactor this stuff
Checklist