Re: [PR] [FLINK-34209] Migrate FileSink to the new SinkV2 API [flink]
Jiabao-Sun commented on code in PR #24180:
URL: https://github.com/apache/flink/pull/24180#discussion_r1467184292
##
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/sink/writer/TestSinkInitContext.java:
##
@@ -48,7 +48,7 @@
import java.util.concurrent.ScheduledFuture;
/** A mock implementation of a {@code Sink.InitContext} to be used in sink
unit tests. */
-public class TestSinkInitContext implements Sink.InitContext {
+public class TestSinkInitContext implements WriterInitContext {
Review Comment:
~Hi @pvary, can we revert this change and provide a new class implements
`WriterInitContext`?
It may cause compatibility issues of the test.~
--
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-34209] Migrate FileSink to the new SinkV2 API [flink]
Jiabao-Sun commented on code in PR #24180:
URL: https://github.com/apache/flink/pull/24180#discussion_r1467200868
##
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/sink/writer/TestSinkInitContext.java:
##
@@ -48,7 +48,7 @@
import java.util.concurrent.ScheduledFuture;
/** A mock implementation of a {@code Sink.InitContext} to be used in sink
unit tests. */
-public class TestSinkInitContext implements Sink.InitContext {
+public class TestSinkInitContext implements WriterInitContext {
Review Comment:
```java
@Deprecated
SinkWriter createWriter(InitContext context) throws IOException;
default SinkWriter createWriter(WriterInitContext context)
throws IOException {
return createWriter(new InitContextWrapper(context));
}
```
```java
TestSinkInitContext initContext = new TestSinkInitContext();
SinkWriter writer = sink.createWriter(initContext);
```
One solution is to not directly construct a Writer through
TestSinkInitContext, but instead use Sink.createWriter() to create a Writer.
So there is no need to revert it change.
Sorry for the disturbance.
--
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-34209] Migrate FileSink to the new SinkV2 API [flink]
Jiabao-Sun commented on code in PR #24180:
URL: https://github.com/apache/flink/pull/24180#discussion_r1467184292
##
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/sink/writer/TestSinkInitContext.java:
##
@@ -48,7 +48,7 @@
import java.util.concurrent.ScheduledFuture;
/** A mock implementation of a {@code Sink.InitContext} to be used in sink
unit tests. */
-public class TestSinkInitContext implements Sink.InitContext {
+public class TestSinkInitContext implements WriterInitContext {
Review Comment:
Hi @pvary, can we revert this change and provide a new class implements
`WriterInitContext`?
It may cause compatibility issues of the test.
--
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-34209] Migrate FileSink to the new SinkV2 API [flink]
gyfora merged PR #24180: URL: https://github.com/apache/flink/pull/24180 -- 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-34209] Migrate FileSink to the new SinkV2 API [flink]
pvary commented on code in PR #24180:
URL: https://github.com/apache/flink/pull/24180#discussion_r1465962747
##
flink-connectors/flink-connector-files/src/main/java/org/apache/flink/connector/file/sink/FileSink.java:
##
@@ -141,13 +145,18 @@ private FileSink(BucketsBuilder> bucketsBuil
}
@Override
-public FileWriter createWriter(InitContext context) throws IOException
{
+public SinkWriter createWriter(InitContext context) throws IOException
{
+throw new UnsupportedOperationException("Not supported");
+}
Review Comment:
This one is needed until we remove the method the deprecated method from the
`Sink` interface.
We needed to keep this for backward compatibility, so if someone used the
`Sink` as a functional interface then their code should keep working.
--
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-34209] Migrate FileSink to the new SinkV2 API [flink]
gyfora commented on code in PR #24180:
URL: https://github.com/apache/flink/pull/24180#discussion_r1465937057
##
flink-connectors/flink-connector-files/src/main/java/org/apache/flink/connector/file/sink/FileSink.java:
##
@@ -141,13 +145,18 @@ private FileSink(BucketsBuilder> bucketsBuil
}
@Override
-public FileWriter createWriter(InitContext context) throws IOException
{
+public SinkWriter createWriter(InitContext context) throws IOException
{
+throw new UnsupportedOperationException("Not supported");
+}
Review Comment:
Do we need to keep this implementation?
--
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-34209] Migrate FileSink to the new SinkV2 API [flink]
gyfora commented on code in PR #24180:
URL: https://github.com/apache/flink/pull/24180#discussion_r1465933942
##
flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/sink/ArrayListAsyncSink.java:
##
@@ -56,6 +58,16 @@ public ArrayListAsyncSink(
maxRecordSizeInBytes);
}
+@Override
+public SinkWriter createWriter(WriterInitContext context) throws
IOException {
+return createWriter(new InitContextWrapper(context));
+}
+
+/**
+ * Should be removed along {@link
+ * org.apache.flink.api.connector.sink2.StatefulSink.StatefulSinkWriter}.
+ */
+@Deprecated
Review Comment:
This is a test sink, should we simply replace the implementation?
--
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-34209] Migrate FileSink to the new SinkV2 API [flink]
pvary commented on PR #24180: URL: https://github.com/apache/flink/pull/24180#issuecomment-1908807413 @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-34209] Migrate FileSink to the new SinkV2 API [flink]
pvary commented on PR #24180: URL: https://github.com/apache/flink/pull/24180#issuecomment-1907753610 The `AsyncFlinkWriter` changes are not covered by the FLIP-372, but they are need to follow the same deprecation patterns for the writer interface so the API is consistent. -- 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-34209] Migrate FileSink to the new SinkV2 API [flink]
pvary commented on code in PR #24180: URL: https://github.com/apache/flink/pull/24180#discussion_r1464617858 ## flink-connectors/flink-connector-base/src/main/java/org/apache/flink/connector/base/sink/writer/AsyncSinkWriter.java: ## @@ -307,6 +313,14 @@ public AsyncSinkWriter( initializeState(states); } +public AsyncSinkWriter( Review Comment: The `AsyncSinkWriter` needs to follow the changes of FLIP-372, so if we remove the deprecated methods, then AsyncSinkWriter could still work -- 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-34209] Migrate FileSink to the new SinkV2 API [flink]
flinkbot commented on PR #24180: URL: https://github.com/apache/flink/pull/24180#issuecomment-1906396145 ## CI report: * 568fa38ead32fd9757def6e462eb4e6680a5feaf 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
[PR] [FLINK-34209] Migrate FileSink to the new SinkV2 API [flink]
pvary opened a new pull request, #24180: URL: https://github.com/apache/flink/pull/24180 ## What is the purpose of the change Currently `FileSink` uses `TwoPhaseCommittingSink` and `StatefulSink` from the SinkV2 API. We should migrate it to use the new FLIP-372 SinkV2 API. There are some additional changes to use the same pattern for the `Deprecated` methods/classes. ## Brief change log Move to the new API. ## Verifying this change Tests are updated where needed. The other tests should cover the existing code ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): no - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no - The serializers: no - The runtime per-record code paths (performance sensitive): no - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no - The S3 file system connector: no ## Documentation - Does this pull request introduce a new feature? no - If yes, how is the feature documented? not applicable -- 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
