[GitHub] [beam] nbali commented on a change in pull request #16909: Introducing KafkaIO.Read implementation compatibility testing

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

nbali commented on a change in pull request #16909:
URL: https://github.com/apache/beam/pull/16909#discussion_r816368169



##########
File path: sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java
##########
@@ -1354,17 +1356,29 @@ private boolean runnerRequiresLegacyRead(PipelineOptions options) {
       }
     }
 
-    private static class ReadFromKafkaViaUnbounded<K, V>
+    private abstract static class AbstractReadFromKafka<K, V>
         extends PTransform<PBegin, PCollection<KafkaRecord<K, V>>> {
       Read<K, V> kafkaRead;
       Coder<K> keyCoder;
       Coder<V> valueCoder;
 
-      ReadFromKafkaViaUnbounded(Read<K, V> kafkaRead, Coder<K> keyCoder, Coder<V> valueCoder) {
+      AbstractReadFromKafka(
+          Read<K, V> kafkaRead,
+          Coder<K> keyCoder,
+          Coder<V> valueCoder,
+          KafkaIOReadImplementation implementation) {
+        KafkaIOReadImplementationCompatibility.getCompatibility(kafkaRead)
+            .checkSupport(implementation);
         this.kafkaRead = kafkaRead;
         this.keyCoder = keyCoder;
         this.valueCoder = valueCoder;
       }
+    }
+
+    static class ReadFromKafkaViaUnbounded<K, V> extends AbstractReadFromKafka<K, V> {

Review comment:
       Same input, same output, same functionality, same fields, same caller. Even without the newly introduced extra shared functionality (`Compatibility.checkSupport`) that screams inheritance to me. Obviously I can replace that if that's the convention here, but strictly theoretically speaking IMO the previous implementation was/would be harder to grasp and this actually makes more sense. When you jump around multiple ptransforms and dofns with barely different names, but (almost) the same fields - like I did during this PR - type/class hierarchy and shared call hierarchy is a big help... but as I said, sure I can duplicate the code so they will be independent ptransforms again.




-- 
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