[GitHub] [beam] rvballada commented on a diff in pull request #17163: [BEAM-13608] JmsIO dynamic topics feature

[GitBox <gi...@apache.org>]

rvballada commented on code in PR #17163:
URL: https://github.com/apache/beam/pull/17163#discussion_r847084544


##########
sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java:
##########
@@ -616,21 +628,34 @@ public void close() throws IOException {
 
     abstract @Nullable String getPassword();
 
-    abstract Builder builder();
+    abstract @Nullable SerializableMessageMapper<EventT> getValueMapper();
+
+    abstract @Nullable SerializableFunction<EventT, String> getTopicNameMapper();
+
+    abstract @Nullable Coder<EventT> getCoder();
+
+    abstract Builder<EventT> builder();
 
     @AutoValue.Builder
-    abstract static class Builder {
-      abstract Builder setConnectionFactory(ConnectionFactory connectionFactory);
+    abstract static class Builder<EventT> {
+      abstract Builder<EventT> setConnectionFactory(ConnectionFactory connectionFactory);
+
+      abstract Builder<EventT> setQueue(String queue);
+
+      abstract Builder<EventT> setTopic(String topic);
+
+      abstract Builder<EventT> setUsername(String username);
 
-      abstract Builder setQueue(String queue);
+      abstract Builder<EventT> setPassword(String password);
 
-      abstract Builder setTopic(String topic);
+      abstract Builder<EventT> setValueMapper(SerializableMessageMapper<EventT> valueMapper);

Review Comment:
   Thinking about the projects already using the legacy "setTopic" method, I think it would be a big breaking change. And introduces a big complexity as a lot of usage would not require dynamic topic use, but just setting the topic name



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org