MINOR: Remove deprecated constructors from Connect's Kafka*BackingStore classes #15865
Conversation
|
Context - #13434 (comment) |
There was a problem hiding this comment.
@yashmayya thanks for this patch. LGTM. just one small comment
| @@ -160,7 +155,7 @@ public KafkaStatusBackingStore(Time time, Converter converter, Supplier<TopicAdm | |||
|
|
|||
| // visible for testing | |||
| KafkaStatusBackingStore(Time time, Converter converter, String statusTopic, KafkaBasedLog<String, byte[]> kafkaLog) { | |||
| this(time, converter); | |||
| this(time, converter, null, "connect-distributed-"); | |||
There was a problem hiding this comment.
This constructor is used by testing. It set topicAdminSupplier to null, so we have to handle the "null" topicAdminSupplier just for testing. That is a bit awkward to me. Could we require those test cases pass a topicAdminSupplier instead of null? Those tests can pass a fake topicAdminSupplier to constructor if they expect topicAdminSupplier should not be called in testing.
There was a problem hiding this comment.
Yeah, that's a good point. I think this and the "ownTopicAdmin" are cruft left over from earlier refactors and are definitely no longer used anywhere in Connect itself (or MM2). Since these aren't part of Connect's public API, I think we should be fine with removing them.
|
@yashmayya any update? I'm ok to merge it and open follow-up to address remaining comments. |
yashmayya
force-pushed
the
remove-deprecated-constructors
branch
from
2631cc6
to
96c9f66
Compare
…wnTopicAdmin' cruft from Kafka*BackingStore classes
yashmayya
force-pushed
the
remove-deprecated-constructors
branch
from
96c9f66
to
73d82d0
Compare
| @@ -160,7 +155,7 @@ public KafkaStatusBackingStore(Time time, Converter converter, Supplier<TopicAdm | |||
|
|
|||
| // visible for testing | |||
| KafkaStatusBackingStore(Time time, Converter converter, String statusTopic, KafkaBasedLog<String, byte[]> kafkaLog) { | |||
| this(time, converter); | |||
| this(time, converter, null, "connect-distributed-"); | |||
There was a problem hiding this comment.
Yeah, that's a good point. I think this and the "ownTopicAdmin" are cruft left over from earlier refactors and are definitely no longer used anywhere in Connect itself (or MM2). Since these aren't part of Connect's public API, I think we should be fine with removing them.
…re classes (apache#15865) - These constructors were deprecated over 3 years ago in KAFKA-10021: Changed Kafka backing stores to use shared admin client to get end offsets and create topics apache#9780. - While these classes are not a part of Connect's public API, deprecation was still introduced instead of outright removal because they are useful utility classes that might've been used outside of Connect. - The KafkaOffsetBackingStore's deprecated constructor was removed in KAFKA-14785: Connect offset read REST API apache#13434. - This patch removes the deprecated constructors for KafkaConfigBackingStore and KafkaStatusBackingStore. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
KafkaOffsetBackingStore's deprecated constructor was removed in KAFKA-14785: Connect offset read REST API #13434.KafkaConfigBackingStoreandKafkaStatusBackingStore.