-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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-32670] Cascade deprecation to classes that implement SourceFunction #23079
Conversation
cc4b4ed
to
de09e34
Compare
de09e34
to
c97f78e
Compare
@flinkbot run azure |
fd464bc
to
85cead2
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.
Thanks @afedulov for the contribution, the changes generally looks good to me, I only left minor comments to polish the PR.
DataStreamSource<List<Float>> result = | ||
env.fromSource(generatorSource, WatermarkStrategy.noWatermarks(), "Vectors Source"); | ||
|
||
result.print(); |
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.
redundant call
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 noticing. I cannot test with CUDA and had to resort to "print"-testing. Fixed.
String kafkaTopic = params.get("kafka-topic"); | ||
String brokers = params.get("brokers", "localhost:9092"); | ||
final StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment(); | ||
env.setParallelism(1); |
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.
Is there any limitation we need to change from default 4 to 1?
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.
That's an accidental leftover from manual testing the rps calculation, thanks for spotting.
@@ -62,7 +66,7 @@ public static void main(String[] args) throws Exception { | |||
// ---- print some usage help ---- | |||
|
|||
System.out.println( | |||
"Usage with built-in data generator: StateMachineExample [--error-rate <probability-of-invalid-transition>] [--sleep <sleep-per-record-in-ms>]"); | |||
"Usage with built-in data generator: StateMachineExample [--error-rate <probability-of-invalid-transition>] [--sleep <sleep-per-record-in-ms> | --rps <records-per-second>]"); |
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.
how about remove legacy --sleep <sleep-per-record-in-ms>
?
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's probably better to keep backwards compatibility for users' sake, it does not actually "cost" us anything.
|
||
@Override | ||
public void run(SourceContext<Tuple2<Long, Long>> ctx) throws Exception { | ||
private static class DataGeneratorFunction |
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.
Add a javadoc for class?
<dependency> | ||
<groupId>org.apache.flink</groupId> | ||
<artifactId>flink-shaded-jackson</artifactId> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.flink</groupId> | ||
<artifactId>flink-shaded-jackson</artifactId> | ||
</dependency> | ||
|
||
<!-- test dependencies --> | ||
<!-- test dependencies --> |
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.
please revert tUnnecessary changes ?
<!-- required by the shade plugin --> | ||
<optional>${flink.markBundledAsOptional}</optional> | ||
</dependency> |
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.
minor: please keep align.
Types.POJO(Event.class)); | ||
TypeInformation.of(Event.class)); |
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.
minor: we can remove this change to above commit and thus let this commit clean
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.
Sure. Since we are OK with shading the datagen, I was planning to roll this commit up into one of the above anyway (marked as [tmp])
@@ -341,6 +343,19 @@ under the License. | |||
<groupId>org.apache.maven.plugins</groupId> | |||
<artifactId>maven-shade-plugin</artifactId> | |||
<executions> | |||
<execution> | |||
<phase>package</phase> |
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.
Shading data-gen
connector to flink-examples-streaming is necessary and it makes sense to me. minor: We can add a meaningful <id>
to describe the shade purpose
85cead2
to
788e7fa
Compare
788e7fa
to
737f4d7
Compare
Thanks @leonardBang , I addressed your commends. |
2c5fe0b
to
95c6690
Compare
Thanks @afedulov for the update, LGTM, but the compile failed, please check it |
95c6690
to
82ffce8
Compare
@flinkbot run azure |
CI is green again. |
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 @afedulov for the contribution, LGTM
…iler checks to Source API This closes apache#23079.
This is a trivial change that marks sub-classes/interfaces of SourceFunction as
@Deprecated
and adjusts some examples to fix the strict deprecation complier check failure.