Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
gyfora merged PR #24080: URL: https://github.com/apache/flink/pull/24080 -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
pvary commented on PR #24080: URL: https://github.com/apache/flink/pull/24080#issuecomment-1893114512 @gyfora: Please merge. We have a green test run. Thx -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
flinkbot commented on PR #24080: URL: https://github.com/apache/flink/pull/24080#issuecomment-174136 ## CI report: * ee54a07048685165fb17e3c57ebcd968e959deac UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
gyfora commented on PR #24022: URL: https://github.com/apache/flink/pull/24022#issuecomment-148837 @dmvk we reverted that commit, seems like there was some mess-up with the passing CI / rebasing. Sorry about that -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
gyfora commented on PR #24022: URL: https://github.com/apache/flink/pull/24022#issuecomment-125329 Thanks @dmvk strange as we had a passed CI run, @pvary is taking a look -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
dmvk commented on PR #24022: URL: https://github.com/apache/flink/pull/24022#issuecomment-113784 https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=56312=results -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
dmvk commented on PR #24022: URL: https://github.com/apache/flink/pull/24022#issuecomment-111191 @pvary @gyfora This PR seems to break master. https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=56314=logs=52b61abe-a3cc-5bde-cc35-1bbe89bb7df5=54421a62-0c80-5aad-3319-094ff69180bb=2540 -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
gyfora merged PR #24022: URL: https://github.com/apache/flink/pull/24022 -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
pvary commented on PR #24022: URL: https://github.com/apache/flink/pull/24022#issuecomment-1887173765 @flinkbot run azure -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
pvary commented on PR #24022: URL: https://github.com/apache/flink/pull/24022#issuecomment-1884867875 @flinkbot run azure -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
pvary commented on PR #24022: URL: https://github.com/apache/flink/pull/24022#issuecomment-1883346813 @flinkbot run azure -- 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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
gyfora commented on code in PR #24022:
URL: https://github.com/apache/flink/pull/24022#discussion_r1445995064
##
flink-core/src/main/java/org/apache/flink/api/connector/sink2/StatefulSink.java:
##
@@ -36,49 +34,43 @@
* @param The type of the sink writer's state
*/
@PublicEvolving
-public interface StatefulSink extends Sink {
+@Deprecated
+public interface StatefulSink
+extends Sink, SupportsWriterState {
/**
- * Create a {@link StatefulSinkWriter}.
+ * Create a {@link
org.apache.flink.api.connector.sink2.StatefulSinkWriter} from a recovered
+ * state.
*
* @param context the runtime context.
* @return A sink writer.
* @throws IOException for any failure during creation.
*/
-StatefulSinkWriter createWriter(InitContext context)
throws IOException;
Review Comment:
As this is a required change to move forward and was accepted as part of the
Flip, it is good to go from my end.
I don't expect this to cause any problems in the Flink / connector ecosystem
--
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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
pvary commented on code in PR #24022:
URL: https://github.com/apache/flink/pull/24022#discussion_r1445985861
##
flink-core/src/main/java/org/apache/flink/api/connector/sink2/StatefulSink.java:
##
@@ -36,49 +34,43 @@
* @param The type of the sink writer's state
*/
@PublicEvolving
-public interface StatefulSink extends Sink {
+@Deprecated
+public interface StatefulSink
+extends Sink, SupportsWriterState {
/**
- * Create a {@link StatefulSinkWriter}.
+ * Create a {@link
org.apache.flink.api.connector.sink2.StatefulSinkWriter} from a recovered
+ * state.
*
* @param context the runtime context.
* @return A sink writer.
* @throws IOException for any failure during creation.
*/
-StatefulSinkWriter createWriter(InitContext context)
throws IOException;
Review Comment:
Oh.. I get it.
If someone expect the writer to be a SinkWriter, they need to cast it after
this change...
That's a valid point. OTOH, this change is really needed to make the API
evolvable and accepted in FLIP-372.
What should we do?
--
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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
gyfora commented on code in PR #24022:
URL: https://github.com/apache/flink/pull/24022#discussion_r1445983265
##
flink-core/src/main/java/org/apache/flink/api/connector/sink2/StatefulSink.java:
##
@@ -36,49 +34,43 @@
* @param The type of the sink writer's state
*/
@PublicEvolving
-public interface StatefulSink extends Sink {
+@Deprecated
+public interface StatefulSink
+extends Sink, SupportsWriterState {
/**
- * Create a {@link StatefulSinkWriter}.
+ * Create a {@link
org.apache.flink.api.connector.sink2.StatefulSinkWriter} from a recovered
+ * state.
*
* @param context the runtime context.
* @return A sink writer.
* @throws IOException for any failure during creation.
*/
-StatefulSinkWriter createWriter(InitContext context)
throws IOException;
Review Comment:
except if someone directly uses the `createWriter` method of the
`StatefulSink` class and expects it to return the `StatefulSinkWriter` subclass
instead of `SinkWriter` right?
--
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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
gyfora commented on code in PR #24022:
URL: https://github.com/apache/flink/pull/24022#discussion_r1445983569
##
flink-core/src/main/java/org/apache/flink/api/connector/sink2/StatefulSink.java:
##
@@ -36,49 +34,43 @@
* @param The type of the sink writer's state
*/
@PublicEvolving
-public interface StatefulSink extends Sink {
+@Deprecated
+public interface StatefulSink
+extends Sink, SupportsWriterState {
/**
- * Create a {@link StatefulSinkWriter}.
+ * Create a {@link
org.apache.flink.api.connector.sink2.StatefulSinkWriter} from a recovered
+ * state.
*
* @param context the runtime context.
* @return A sink writer.
* @throws IOException for any failure during creation.
*/
-StatefulSinkWriter createWriter(InitContext context)
throws IOException;
Review Comment:
not sure if this is a big issue but we should at least make a note of it
--
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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
pvary commented on code in PR #24022:
URL: https://github.com/apache/flink/pull/24022#discussion_r1445981804
##
flink-core/src/main/java/org/apache/flink/api/connector/sink2/StatefulSink.java:
##
@@ -36,49 +34,43 @@
* @param The type of the sink writer's state
*/
@PublicEvolving
-public interface StatefulSink extends Sink {
+@Deprecated
+public interface StatefulSink
+extends Sink, SupportsWriterState {
/**
- * Create a {@link StatefulSinkWriter}.
+ * Create a {@link
org.apache.flink.api.connector.sink2.StatefulSinkWriter} from a recovered
+ * state.
*
* @param context the runtime context.
* @return A sink writer.
* @throws IOException for any failure during creation.
*/
-StatefulSinkWriter createWriter(InitContext context)
throws IOException;
Review Comment:
`StatefulSink` still inherits from `Sink` which has this deprecated method
which will take its place:
```
@Deprecated
SinkWriter createWriter(InitContext context) throws IOException;
```
So I think removing this is not a breaking change. @gyfora: Do I miss
something?
Thanks,
Peter
--
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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
gyfora commented on code in PR #24022:
URL: https://github.com/apache/flink/pull/24022#discussion_r1445948831
##
flink-core/src/main/java/org/apache/flink/api/connector/sink2/StatefulSink.java:
##
@@ -36,49 +34,43 @@
* @param The type of the sink writer's state
*/
@PublicEvolving
-public interface StatefulSink extends Sink {
+@Deprecated
+public interface StatefulSink
+extends Sink, SupportsWriterState {
/**
- * Create a {@link StatefulSinkWriter}.
+ * Create a {@link
org.apache.flink.api.connector.sink2.StatefulSinkWriter} from a recovered
+ * state.
*
* @param context the runtime context.
* @return A sink writer.
* @throws IOException for any failure during creation.
*/
-StatefulSinkWriter createWriter(InitContext context)
throws IOException;
Review Comment:
is it okay to remove this method? Technically there could be scenarios where
this would be a breaking change.
--
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
Re: [PR] [FLINK-33973] Add new interfaces for SinkV2 to synchronize the API with the SourceV2 API [flink]
flinkbot commented on PR #24022: URL: https://github.com/apache/flink/pull/24022#issuecomment-1875319278 ## CI report: * 922c6c60c3b8d66f925d9bc84855b4beaaa90359 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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
