lindong28 commented on a change in pull request #18397:
URL: https://github.com/apache/flink/pull/18397#discussion_r789704887
##########
File path:
flink-connectors/flink-connector-kafka/src/main/java/org/apache/flink/connector/kafka/sink/KafkaSerializerWrapper.java
##########
@@ -65,8 +77,15 @@ public void open(InitializationContext context) throws
Exception {
serializerClass.getName(),
Serializer.class,
getClass().getClassLoader());
+
+ if (config.isEmpty()) {
Review comment:
It is possible that relying on an invocation of `configure(...)` with an
empty config is a bad practice. However, since this is an existing API that
public users might already be using, users will be unhappy if our change breaks
their existing application, right? I am just not sure it is a good idea to tell
users that we break their application because we believe their usage is wrong.
Also, the public method will probably be easier to understand/use if its
semantics is `deserializer::configure(...) will be invoked with the config map
provided`, rather than saying `deserializer::configure(...) will be invoked
with the config map provided if the map is empty`. 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
