pvary commented on code in PR #23555:
URL: https://github.com/apache/flink/pull/23555#discussion_r1371740397
##########
flink-core/src/main/java/org/apache/flink/api/connector/sink2/TwoPhaseCommittingSink.java:
##########
@@ -77,4 +97,42 @@ interface PrecommittingSinkWriter<InputT, CommT> extends
SinkWriter<InputT> {
*/
Collection<CommT> prepareCommit() throws IOException,
InterruptedException;
}
+
+ /** The interface exposes some runtime info for creating a {@link
Committer}. */
+ @PublicEvolving
+ interface CommitterInitContext {
Review Comment:
To make the deprecation work, I need to have these implementations:
- Default implementation for the old method - so the new implementations
does not need to implement it
```
@Deprecated
default SinkWriter<InputT> createWriter(InitContext context) throws
IOException {
throw new UnsupportedOperationException("Please implement
createWriter(WriterInitContext context)");
}
```
- Default implementation for the new method - so the old implementations are
not broken
```
default SinkWriter<InputT> createWriter(WriterInitContext context)
throws IOException {
return createWriter(new InitContextWrapper(context));
}
```
Sadly there is a bit of confusing inheritance hierarchy where the return
type of the `createWriter` method is narrowed down by the `StatefulSink`
(`StatefulSinkWriter`), and `TwoPhaseCommittingSink`
(`PrecommittingSinkWriter`). If I do not want to change the return type of the
methods, then I need specific default implementations for these methods which
will clash when there is a Sink which implements both interfaces. I got the
following compile error:
>
org.apache.flink.streaming.runtime.operators.sink.TestSinkV2.TestStatefulSinkV2
inherits unrelated defaults for createWriter(InitContext) from types
org.apache.flink.api.connector.sink2.TwoPhaseCommittingSink and
org.apache.flink.api.connector.sink2.StatefulSink
**So providing a seamless upgrade with default implementation is not a
viable option**, and for Option 2 we have to break the API. This is not a big
change (the user must change the parameter type of the method from
`InitContext` to `WriterInitContext`), but a change never less.
Also if we decide on accepting the breaking change, we might accept the
breaking change for the `createCommitter` method as well (adding a new
parameter). This could simplify the code again, since we do not need deprecated
methods, just the pure implementation. This is again not a big change from the
user side, but would require a code change from the user.
@tzulitai: What do you think?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
