Poorvankbhatia commented on code in PR #26274:
URL: https://github.com/apache/flink/pull/26274#discussion_r2025421806
##########
flink-connectors/flink-connector-base/src/main/java/org/apache/flink/connector/base/sink/writer/AsyncSinkWriter.java:
##########
@@ -213,11 +213,26 @@ protected void submitRequestEntries(
*/
protected abstract long getSizeInBytes(RequestEntryT requestEntry);
+ /**
+ * This constructor is deprecated. Users should use {@link
#AsyncSinkWriter(ElementConverter,
+ * WriterInitContext, AsyncSinkWriterConfiguration, Collection,
BatchCreator, BufferWrapper)}.
+ */
+ @Deprecated
public AsyncSinkWriter(
ElementConverter<InputT, RequestEntryT> elementConverter,
WriterInitContext context,
AsyncSinkWriterConfiguration configuration,
Collection<BufferedRequestState<RequestEntryT>> states) {
+ this(elementConverter, context, configuration, states, null, null);
+ }
+
+ public AsyncSinkWriter(
+ ElementConverter<InputT, RequestEntryT> elementConverter,
+ WriterInitContext context,
+ AsyncSinkWriterConfiguration configuration,
+ Collection<BufferedRequestState<RequestEntryT>> states,
+ @Nullable BatchCreator<RequestEntryT> batchCreator,
+ @Nullable BufferWrapper<RequestEntryT> bufferedRequestEntries) {
Review Comment:
Yeah, I thought about it. Moving batchCreator and bufferWrapper into
AsyncSinkWriterConfiguration does help with backward compatibility and
simplifies usage for sink implementors.
The only hesitation I had was around type safety. Since
AsyncSinkWriterConfiguration would store them as:
```
private BatchCreator<?> batchCreator;
private BufferWrapper<?> bufferWrapper;
```
and i would need to cast them when retrieving (the casting is needed because
AsyncSinkWriterConfiguration isn't generic and doesn't know about
RequestEntryT):
```
this.batchCreator = (BatchCreator<RequestEntryT>)
configuration.getBatchCreator();
this.bufferWrapper = (BufferWrapper<RequestEntryT>)
configuration.getBufferWrapper();
```
This means we lose compile-time type guarantees at the point of usage inside
AsyncSinkWriter. If someone accidentally wires a mismatched generic type, we
wouldn't catch it at compile time—it would fail at runtime. Let me know if you
think there is a better approach, as i was under the impression that
AsyncSinkWriterConfiguration is meant to be a non-generic configuration class.
So i have marked the oder constructor as `deprecated` but that internally calls
this constructor with default values, so till it is removed sink implementors
might not be required to change the code.
--
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]
