You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/04/08 00:50:27 UTC
[GitHub] [kafka] guozhangwang opened a new pull request #10502: KAFKA-12630: Remove deprecated KafkaClientSupplier#getAdminClient in Streams
guozhangwang opened a new pull request #10502:
URL: https://github.com/apache/kafka/pull/10502
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10502: KAFKA-12630: Remove deprecated KafkaClientSupplier#getAdminClient in Streams
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10502:
URL: https://github.com/apache/kafka/pull/10502#discussion_r611064000
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaClientSupplier.java
##########
@@ -31,29 +30,14 @@
* @see KafkaStreams#KafkaStreams(Topology, java.util.Properties, KafkaClientSupplier)
*/
public interface KafkaClientSupplier {
- /**
- * Create an {@link AdminClient} which is used for internal topic management.
- *
- * @param config Supplied by the {@link java.util.Properties} given to the {@link KafkaStreams}
- * @return an instance of {@link AdminClient}
- * @deprecated Not called by Kafka Streams, which now uses {@link #getAdmin} instead.
- */
- @Deprecated
- default AdminClient getAdminClient(final Map<String, Object> config) {
- throw new UnsupportedOperationException("Direct use of this method is deprecated. " +
- "Implementations of KafkaClientSupplier should implement the getAdmin() method instead. " +
- "The method will be removed in a future release.");
- }
-
/**
* Create an {@link Admin} which is used for internal topic management.
*
* @param config Supplied by the {@link java.util.Properties} given to the {@link KafkaStreams}
* @return an instance of {@link Admin}
*/
- @SuppressWarnings("deprecation")
default Admin getAdmin(final Map<String, Object> config) {
- return getAdminClient(config);
+ throw new UnsupportedOperationException("Implementations of KafkaClientSupplier should implement the getAdmin() method.");
Review comment:
Would we want to remove this default implementation too? Not saying we should, but what's the benefit?
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaClientSupplier.java
##########
@@ -31,29 +30,14 @@
* @see KafkaStreams#KafkaStreams(Topology, java.util.Properties, KafkaClientSupplier)
*/
public interface KafkaClientSupplier {
- /**
- * Create an {@link AdminClient} which is used for internal topic management.
- *
- * @param config Supplied by the {@link java.util.Properties} given to the {@link KafkaStreams}
- * @return an instance of {@link AdminClient}
- * @deprecated Not called by Kafka Streams, which now uses {@link #getAdmin} instead.
- */
- @Deprecated
- default AdminClient getAdminClient(final Map<String, Object> config) {
- throw new UnsupportedOperationException("Direct use of this method is deprecated. " +
- "Implementations of KafkaClientSupplier should implement the getAdmin() method instead. " +
- "The method will be removed in a future release.");
- }
-
/**
* Create an {@link Admin} which is used for internal topic management.
*
* @param config Supplied by the {@link java.util.Properties} given to the {@link KafkaStreams}
* @return an instance of {@link Admin}
*/
- @SuppressWarnings("deprecation")
default Admin getAdmin(final Map<String, Object> config) {
- return getAdminClient(config);
+ throw new UnsupportedOperationException("Implementations of KafkaClientSupplier should implement the getAdmin() method.");
Review comment:
Would we want to remove this default implementation too? Not saying we should, but what's the benefit of keeping it?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #10502: KAFKA-12630: Remove deprecated KafkaClientSupplier#getAdminClient in Streams
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #10502:
URL: https://github.com/apache/kafka/pull/10502#issuecomment-815368022
ping @ableegoldman @abbccdda
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang merged pull request #10502: KAFKA-12630: Remove deprecated KafkaClientSupplier#getAdminClient in Streams
Posted by GitBox <gi...@apache.org>.
guozhangwang merged pull request #10502:
URL: https://github.com/apache/kafka/pull/10502
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on a change in pull request #10502: KAFKA-12630: Remove deprecated KafkaClientSupplier#getAdminClient in Streams
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #10502:
URL: https://github.com/apache/kafka/pull/10502#discussion_r615155809
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaClientSupplier.java
##########
@@ -31,29 +30,14 @@
* @see KafkaStreams#KafkaStreams(Topology, java.util.Properties, KafkaClientSupplier)
*/
public interface KafkaClientSupplier {
- /**
- * Create an {@link AdminClient} which is used for internal topic management.
- *
- * @param config Supplied by the {@link java.util.Properties} given to the {@link KafkaStreams}
- * @return an instance of {@link AdminClient}
- * @deprecated Not called by Kafka Streams, which now uses {@link #getAdmin} instead.
- */
- @Deprecated
- default AdminClient getAdminClient(final Map<String, Object> config) {
- throw new UnsupportedOperationException("Direct use of this method is deprecated. " +
- "Implementations of KafkaClientSupplier should implement the getAdmin() method instead. " +
- "The method will be removed in a future release.");
- }
-
/**
* Create an {@link Admin} which is used for internal topic management.
*
* @param config Supplied by the {@link java.util.Properties} given to the {@link KafkaStreams}
* @return an instance of {@link Admin}
*/
- @SuppressWarnings("deprecation")
default Admin getAdmin(final Map<String, Object> config) {
- return getAdminClient(config);
+ throw new UnsupportedOperationException("Implementations of KafkaClientSupplier should implement the getAdmin() method.");
Review comment:
I think we can; I was originally worried if I change it from `default` to a pure interface would that be binary compatible --- did some research and it seems okay.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org