-
Notifications
You must be signed in to change notification settings - Fork 13k
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-8149][kinesis] Replace usages of deprecated SerializationSchema #5069
Conversation
@@ -41,7 +42,6 @@ | |||
import org.apache.flink.streaming.connectors.kinesis.serialization.KinesisDeserializationSchema; | |||
import org.apache.flink.streaming.connectors.kinesis.serialization.KinesisDeserializationSchemaWrapper; | |||
import org.apache.flink.streaming.connectors.kinesis.util.KinesisConfigUtil; | |||
import org.apache.flink.streaming.util.serialization.DeserializationSchema; |
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 think we can not simply remove the usage of this, as that will be an user API-breaking change (for users that use the FlinkKinesisConsumer(String, DeserializationSchema<T>, Properties)
constructor).
Instead of removing this, we should deprecate the constructors that accept the deprecated DeserializationSchema
interface, and add new corresponding constructors that accept the new migrated DeserializationSchema
.
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.
make sense.
import org.apache.flink.configuration.Configuration; | ||
import org.apache.flink.runtime.state.FunctionInitializationContext; | ||
import org.apache.flink.runtime.state.FunctionSnapshotContext; | ||
import org.apache.flink.streaming.api.checkpoint.CheckpointedFunction; | ||
import org.apache.flink.streaming.api.functions.sink.RichSinkFunction; | ||
import org.apache.flink.streaming.connectors.kinesis.serialization.KinesisSerializationSchema; | ||
import org.apache.flink.streaming.connectors.kinesis.util.KinesisConfigUtil; | ||
import org.apache.flink.streaming.util.serialization.SerializationSchema; |
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.
Same here; see above coment.
Thanks for the PR @yew1eb! I have one comment regarding how the change breaks user API. |
@tzulitai, thanks for your review. I have updated the PR according to your comments. |
LGTM, merging this .. |
Ah, I just realized that my comment on the last review is irrelevant, since Sorry about this. The PR is good to go without the extra follow-up fix. Merging the first commit. |
I just saw the code in src tree and traced back to this PR. I feel it's really bad to have runnable examples (with java main method) in a lib jar that Flink distributes. Can we move the examples to |
@bowenli86 do you mean the |
My apologies! Silly mistake at late night when the brain is quite inactive.... I forgot to check the files history, and thought this PR (which shows as latest commit) added those examples... Opened FLINK-8218. Sincere apologies again! |
What is the purpose of the change
The deprecated
SerializationSchema
inflink-streaming-java
, has been moved toflink-core
.replace usages of deprecated
SerializationSchema
inflink-connector-kinesis
.Brief change log
DeserializationSchema
,SerializationSchema
,SimpleStringSchema
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:
@Public(Evolving)
: (yes / no)Documentation