[GitHub] [kafka] vvcephei commented on a change in pull request #8864: KAFKA-9274: Mark `retries` config as deprecated and add new `task.timeout.ms` config

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

vvcephei commented on a change in pull request #8864:
URL: https://github.com/apache/kafka/pull/8864#discussion_r450533229



##########
File path: clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java
##########
@@ -662,6 +662,7 @@ public void testInvalidTopicNames() throws Exception {
         }
     }
 
+    @SuppressWarnings("deprecation")

Review comment:
       Thanks @mjsax. I agree we should keep testing the deprecated option. It does kind of seem like we should rewrite the tests you want to rewrite, now, though. I don't think this scheme of deprecating some tests and suppressing the warning on others is going to be remembered for very long after this is merged.

##########
File path: clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java
##########
@@ -1447,6 +1451,7 @@ public void testListConsumerGroupsWithStatesOlderBrokerVersion() throws Exceptio
         }
     }
 
+    @Deprecated

Review comment:
       I think the deprecation warning was part of a scheme to mark which tests should be rewritten: https://github.com/apache/kafka/pull/8864/files#r439685696

##########
File path: streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java
##########
@@ -159,6 +159,7 @@ public void testGetGroupInstanceIdConfigs() {
         assertNull(returnedProps.get(ConsumerConfig.GROUP_INSTANCE_ID_CONFIG));
     }
 
+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR

Review comment:
       Hey, sorry if this is picky, but are these also to be removed when we remove the config? If so, maybe we should just say it on every suppression.

##########
File path: core/src/main/scala/kafka/tools/ConsoleProducer.scala
##########
@@ -146,7 +148,7 @@ object ConsoleProducer {
       .describedAs("size")
       .ofType(classOf[java.lang.Integer])
       .defaultsTo(200)
-    val messageSendMaxRetriesOpt = parser.accepts("message-send-max-retries", "Brokers can fail receiving the message for multiple reasons, and being unavailable transiently is just one of them. This property specifies the number of retires before the producer give up and drop this message.")
+    val messageSendMaxRetriesOpt = parser.accepts("message-send-max-retries", "(Deprecated) Brokers can fail receiving the message for multiple reasons, and being unavailable transiently is just one of them. This property specifies the number of retires before the producer give up and drop this message.")

Review comment:
       Sounds good to me.

##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/GlobalStateManagerImpl.java
##########
@@ -74,6 +74,7 @@
     private final OffsetCheckpoint checkpointFile;
     private final Map<TopicPartition, Long> checkpointFileCache;
 
+    @SuppressWarnings("deprecation") // TODO: remove in follow up PR when `RETRIES` is removed

Review comment:
       Thanks for adding the comment!

##########
File path: docs/streams/upgrade-guide.html
##########
@@ -86,6 +86,11 @@ <h1>Upgrade Guide and API Changes</h1>
         More details about the new config <code>StreamsConfig#TOPOLOGY_OPTIMIZATION</code> can be found in <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-295%3A+Add+Streams+Configuration+Allowing+for+Optional+Topology+Optimization">KIP-295</a>.
     </p>
 
+    <h3><a id="streams_api_changes_270" href="#streams_api_changes_270">Streams API changes in 2.7.0</a></h3>
+    <p>
+        TODO

Review comment:
       Did you mean to fill this in as part of this PR?




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