-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-36625] add lineage helper class for connector integration #25712
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
[FLINK-36625] add lineage helper class for connector integration #25712
Conversation
8171f17 to
87538a2
Compare
87538a2 to
edbdb1d
Compare
| @PublicEvolving | ||
| public interface TypeDatasetFacet extends LineageDatasetFacet { | ||
|
|
||
| TypeInformation getTypeInformation(); |
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.
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.
Just to note, IMO @Nonnull usage adds a lot of bloat to the code (and Java is bloated enough on its own already) compared to its benefit. Using @Nullable is a lot more practical and reasonable when something can be null in my experience, but in this repo, none of the are (or should be) enforced.
| * Returns a type dataset facet or `Optional.empty` in case an implementing class is not able to | ||
| * resolve type. | ||
| */ | ||
| Optional<TypeDatasetFacet> getTypeDatasetFacet(); |
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 see this is a new interface but it is not referenced in the fix, what is the thinking 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.
This interface is mainly for some connector in which return type is provided by internal class rather than the source/sink directly.
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.
Thanks for the explanation. It would be good to understand when a connector author should use this interface. Could we add documentation around when this and the utility classes could/ should be used ?
| return datasetOf(name, namespace, Collections.singletonList(typeDatasetFacet)); | ||
| } | ||
|
|
||
| public static LineageDataset datasetOf( |
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 wonder if these datasetOf constructor methods can be put in DefaultLineageDataset or a class that creates DefaultLineageDataset , maybe a DefaultLineageDatasetProvider
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.
It is mainly to make the LineageDataset creation easier and simplify the code in each of connectors.
edbdb1d to
7771506
Compare
|
Reviewed by Chi on 05/12/24. Asked submitter questions. Requests documentation be updated on how connector authors should / could use this helper class. |
|
@davidradl |
|
After this PR is merged, I will update this PR #25762. |
|
This PR is being marked as stale since it has not had any activity in the last 90 days. If you are having difficulty finding a reviewer, please reach out to the If this PR is no longer valid or desired, please feel free to close it. |
|
This PR has been closed since it has not had any activity in 120 days. |
|
@flinkbot run azure |
ferenc-csaky
left a comment
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, I'll merge this be tomorrow EOD if CI is green.
|
@ferenc-csaky |
It's fine, these classes were not touched since then at all, no reason to rebase. |
What is the purpose of the change
Add helper classes in stream java api for lineage integration in connectors.
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation