homatthew commented on code in PR #3738:
URL: https://github.com/apache/gobblin/pull/3738#discussion_r1294135399
##########
gobblin-modules/gobblin-kafka-common/src/main/java/org/apache/gobblin/source/extractor/extract/kafka/KafkaExtractor.java:
##########
@@ -337,4 +340,24 @@ public void close() throws IOException {
public long getHighWatermark() {
return 0;
}
+
+ public static String getKafkaBrokerSimpleName(State state) {
Review Comment:
In my mind, this kafka extractor method is already a downstream method that
wants exactly 1 broker. Any implementation of this class that properly supports
multiple kafka brokers, should have its own implementation and work with the
map directly.
This implementation works out of the box for those that are just consuming
from a single kafka broker per extractor, which is IMO the general case and
seems to be the case for existing implementations.
##########
gobblin-modules/gobblin-kafka-common/src/main/java/org/apache/gobblin/source/extractor/extract/kafka/KafkaExtractor.java:
##########
@@ -337,4 +340,24 @@ public void close() throws IOException {
public long getHighWatermark() {
return 0;
}
+
+ public static String getKafkaBrokerSimpleName(State state) {
Review Comment:
I put the annoying boiler plate for parsing the map in the common utils
which we would never want to duplicate, and I think that is the correct layer
of abstraction for all use cases (single kafka broker or multiple).
##########
gobblin-modules/gobblin-kafka-common/src/main/java/org/apache/gobblin/source/extractor/extract/kafka/KafkaExtractor.java:
##########
@@ -337,4 +340,24 @@ public void close() throws IOException {
public long getHighWatermark() {
return 0;
}
+
+ public static String getKafkaBrokerSimpleName(State state) {
Review Comment:
The reason I separated this from the original common utils is because if
someone has multiple kafka brokers, then it wouldn't make much sense to return
the list of strings. Realistically they'd need to map out which simple name
corresponds to which broker uri.
In other words, they would definitely want the entire map so that they it's
easier to reason about in 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]
