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

[FLINK-36245][FOLLOWUP] Remove the @Deprecated for ArrowSourceFunction and SinkFunction #26297

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

beliefer
Copy link
Contributor

What is the purpose of the change

This PR follows up #25331
It seems missing the two classes.

Brief change log

Remove the @deprecated for ArrowSourceFunction and SinkFunction

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 14, 2025

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@beliefer
Copy link
Contributor Author

*/
@Deprecated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To fix the issue.

2025-03-14T14:31:28.5664450Z Mar 14 14:31:28 14:31:28.565 [ERROR] Failures: 
2025-03-14T14:31:28.5666940Z Mar 14 14:31:28 14:31:28.565 [ERROR]   Architecture Violation [Priority: MEDIUM] - Rule 'Return and argument types of methods annotated with @Public must be annotated with @Public.' was violated (2 times):
2025-03-14T14:31:28.5669896Z Mar 14 14:31:28 org.apache.flink.streaming.api.datastream.DataStream.addSink(org.apache.flink.streaming.api.functions.sink.legacy.SinkFunction): Argument leaf type org.apache.flink.streaming.api.functions.sink.legacy.SinkFunction does not satisfy: reside outside of package 'org.apache.flink..' or reside in any package ['..shaded..'] or annotated with @Public or annotated with @Deprecated
2025-03-14T14:31:28.5673341Z Mar 14 14:31:28 org.apache.flink.streaming.api.datastream.KeyedStream.addSink(org.apache.flink.streaming.api.functions.sink.legacy.SinkFunction): Argument leaf type org.apache.flink.streaming.api.functions.sink.legacy.SinkFunction does not satisfy: reside outside of package 'org.apache.flink..' or reside in any package ['..shaded..'] or annotated with @Public or annotated with @Deprecated

@beliefer beliefer force-pushed the FLINK-36245_followup branch from 14433b6 to c3bcbda Compare March 15, 2025 06:38
@@ -301,6 +301,7 @@ protected <R> SingleOutputStreamOperator<R> doTransform(
}

@Override
@Deprecated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To fix the issue.

2025-03-15T03:40:51.6629822Z Mar 15 03:40:51 03:40:51.661 [ERROR] Tests run: 4, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 14.56 s <<< FAILURE! -- in org.apache.flink.architecture.rules.ApiAnnotationRules
2025-03-15T03:40:51.6631130Z Mar 15 03:40:51 03:40:51.662 [ERROR] ApiAnnotationRules.PUBLIC_API_METHODS_USE_ONLY_PUBLIC_API_TYPES -- Time elapsed: 0.118 s <<< FAILURE!
2025-03-15T03:40:51.6631774Z Mar 15 03:40:51 java.lang.AssertionError: 
2025-03-15T03:40:51.6632704Z Mar 15 03:40:51 Architecture Violation [Priority: MEDIUM] - Rule 'Return and argument types of methods annotated with @Public must be annotated with @Public.' was violated (1 times):
2025-03-15T03:40:51.6634529Z Mar 15 03:40:51 org.apache.flink.streaming.api.datastream.KeyedStream.addSink(org.apache.flink.streaming.api.functions.sink.legacy.SinkFunction): Argument leaf type org.apache.flink.streaming.api.functions.sink.legacy.SinkFunction does not satisfy: reside outside of package 'org.apache.flink..' or reside in any package ['..shaded..'] or annotated with @Public or annotated with @Deprecated

Copy link

@Poorvankbhatia Poorvankbhatia left a comment

Choose a reason for hiding this comment

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

Hey @beliefer . We are in the process of Rewriting the Arrow Source Function, i.e moving it to the new Source API. So should we continue this deprecated for now. Here is the ticket
cc: @afedulov wdyt?

@beliefer
Copy link
Contributor Author

@Poorvankbhatia After the rewrite, will the behavior changed? If yes, I think we should continue this deprecated pr.

@Poorvankbhatia
Copy link

@Poorvankbhatia After the rewrite, will the behavior changed? If yes, I think we should continue this deprecated pr.

I think the core functionality of the source should remain the same, but the design is still WIP. So IMO we can keep the @deprecated annotation on ArrowSourceFunction

@beliefer
Copy link
Contributor Author

IMO, I think this PR is not related to the rewrite work.
This PR is just to fix bug that both deprecated with annotation and comment.

@@ -864,7 +864,11 @@ protected DataStream<T> setConnectionType(StreamPartitioner<T> partitioner) {
*
* @param sinkFunction The object containing the sink's invoke function.
* @return The closed DataStream.
* @deprecated This method relies on the {@link SinkFunction} API, which is due to be removed.
Copy link
Contributor

@davidradl davidradl Mar 20, 2025

Choose a reason for hiding this comment

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

I see text "due to be removed." It will not be removed until at least Flink v3.
Idoes not look right to have a deprecated method referencing parameters are annotated with internal

Copy link
Contributor Author

@beliefer beliefer Mar 21, 2025

Choose a reason for hiding this comment

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

Yes. Because the SinkFunction will be removed in future versions, this function depends it.

ApiAnnotationRules lead to the error https://github.com/apache/flink/pull/26297/files#r1996509218
It means we should add @Deprecated

@@ -32,7 +32,6 @@
* org.apache.flink.api.connector.sink2.Sink} interface instead.
*/
@Internal
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we changing this tag?
It is was part of the legacy deprecated API - should this not be deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should still be deprecated. I remove it since the annotation @deprecated duplicate with the comments @deprecated. And I follow up the #25331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants