sharonx commented on code in PR #3775:
URL: https://github.com/apache/flink-cdc/pull/3775#discussion_r1885836067
##########
flink-cdc-composer/src/main/java/org/apache/flink/cdc/composer/PipelineExecution.java:
##########
@@ -27,10 +29,18 @@ public interface PipelineExecution {
class ExecutionInfo {
private final String id;
private final String description;
+ private final JobClient jobClient;
public ExecutionInfo(String id, String description) {
this.id = id;
this.description = description;
+ this.jobClient = null;
Review Comment:
I'd try to avoid `null` usage as much as possible to avoid potential NPEs.
It does look like the `jobClient` is available in all the places that construct
`ExecutionInfo`. So can we just change the constructor? If not, please add
`@Nullable` wherever is applicable.
##########
flink-cdc-composer/src/main/java/org/apache/flink/cdc/composer/flink/FlinkPipelineComposer.java:
##########
@@ -54,6 +54,11 @@ public class FlinkPipelineComposer implements
PipelineComposer {
private final StreamExecutionEnvironment env;
private final boolean isBlocking;
+ public static FlinkPipelineComposer ofStreamExecutionEnvironment(
+ StreamExecutionEnvironment env) {
+ return new FlinkPipelineComposer(env, false);
+ }
Review Comment:
hmm to me this essentially expose the constructor and it's a bit opaque to
set `isBlocking=false` statically here as you can have blocking mode for stream
execution env too. My proposal is to make the constructor below public.
```
private FlinkPipelineComposer(StreamExecutionEnvironment env, boolean
isBlocking) {
this.env = env;
this.isBlocking = isBlocking;
}
```
@PatrickRen I see that you are tagged in the history of the class. Any
opinions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
