KAFKA-9274: Mark `retries` config as deprecated and add new `task.timeout.ms` config by mjsax · Pull Request #8864 · apache/kafka

mjsax · 2020-06-13T00:06:43Z

First PR for KIP-572 for public changes only. Producer/Admin client won't have any functional change as they still will respect retries. For KS, we will get rid or the usage or retries in follow up PRs.

Call for review: @cmccabe @hachikuji @abbccdda @vvcephei

mjsax · 2020-06-13T00:10:41Z

@cmccabe I think we can keep most of those test as-is until retries is removed -- the ones with @SuppressWarning should be rewritten if retries is removed IMHO, while the ones with @Deprecated should be remove. Let me know if this makes sense or if you think we should rewrite those tests right away.

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.

We can do that. Wanted to get input from @hachikuji and @cmccabe about it. Frankly, I am not always 100% sure atm how to exactly rewrite the tests and what test coverage is desired?

I dug into the test and I think I found ways to get rid for retries for the methods that suppress the deprecation warning. Let me know what you think.

mjsax · 2020-06-13T00:12:14Z

@rhauch @kkonstantine -- We deprecate retries via KIP-572. Not sure if we should rewrite this test right away so it does not use retries any longer?

Not sure if we should rewrite this test right away

First of all, this is not test code. It's actually part of the Connect runtime.

Second, I'd have to look into what we would do differently and how we would set deliver.timeout.ms to work with the existing logic that handles retries. Since it's not clear, I suggest we keep the current behavior of setting retries=0 as long as that still function. We're using the ProducerConfig.RETRIES constant, so that can't be removed without changing this code.

@rhauch @kkonstantine As discussed in person, I create a ticket that is marked as blocker for 2.7 release to address this issue: https://issues.apache.org/jira/browse/KAFKA-10297

Btw: @hachikuji raise a concern about this issue, too: #8864 (review)

I just had one question about the proposal. Using retries=0 in the producer allows the user to have "at-most-once" delivery. This allows the application to implement its own retry logic for example. Do we still have a way to do this once this configuration is gone?

So maybe we need to do some follow up work in the Producer to make it work for Connect. But I would defer this to the follow up work.

My original though was, that setting max.deliver.timeout.ms := request .timeout.ms might prevent internal retries. But we need to verify this. It was also brought to my attention, that this might not work if the network disconnects -- only retries=0 would prevent to re-open the connection but a low timeout would not prevent retries.

In KIP-572, we proposed for Kafka Streams itself, to treat task.timeout.ms = 0 as "no retries" -- maybe we can do a similar thing for the producer?

There is also max.block.ms that we should consider. Unfortunately, I am not an expert on the Producer. But I would like to move forward with KIP-572 for now and are happy to help to resolve those questions.

mjsax · 2020-06-13T00:14:20Z

@hachikuji @cmccabe @vvcephei -- I think we should deprecate this flag (guess we should update the KIP accordingly) -- Thoughts?

Sounds good to me.

Updated the KIP and sent a mail to the VOTE thread on the dev list.

mjsax · 2020-06-13T00:16:46Z

Just some side cleanup. Method does not throw anything

abbccdda

Looks like the only remaining question is the deprecation path, and no actual production logic change? If so, I could rely on reviews from other tech leads to unblock.

hachikuji

I just had one question about the proposal. Using retries=0 in the producer allows the user to have "at-most-once" delivery. This allows the application to implement its own retry logic for example. Do we still have a way to do this once this configuration is gone?

hachikuji · 2020-07-06T21:39:02Z

nit: feels like overkill to deprecate test cases. Since they reference RETRIES directly, I don't think we need to worry about them not getting removed

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

Correct. To make the build pass, we either need to add a Suppress annotation of just deprecate the test itself. Deprecating the test itself is "cleaner" IMHO.

Fair enough.

vvcephei

Hey @mjsax , sorry I was so slow on the review. It LGTM overall, just a few comments. Thanks for the PR!

vvcephei · 2020-07-06T23:38:54Z

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.

vvcephei · 2020-07-06T23:39:35Z

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

vvcephei · 2020-07-06T23:40:04Z

Sounds good to me.

vvcephei · 2020-07-06T23:41:10Z

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

vvcephei · 2020-07-06T23:47:06Z

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.

Not 100% sure yet how the test will be rewritten. My expectation is, that we remove the retries config and also remove the Suppress tag -- however, we can only do this, after streams code was changes.

Addressed via #9060

vvcephei · 2020-07-06T23:47:37Z

Thanks for adding the comment!

Yes :) Added it as a reminder only first. Was expecting to get review comment anyway.

Addressed via #9047

mjsax · 2020-07-21T00:12:58Z

Updates this PR.

vvcephei

Thanks for the update, @mjsax !

There are still multiple comments like TODO revisit in follow up PR. Should we file a blocker to actually follow up on this? Otherwise, I'm afraid we'll just forget about them. If I understand the change correctly, these tests are all using the deprecated retries parameter, which means we're lacking coverage on the non-deprecated path.

Otherwise, it LGTM!

mjsax · 2020-07-21T19:15:42Z

Thanks @vvcephei -- I don't think we need tickets. I will use this PR as reference itself to make sure we address all TODO that are added.

mjsax · 2020-07-21T19:46:54Z

        }
    }

+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Addressed via #9047

mjsax · 2020-07-21T19:47:02Z

        }
    }

+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Addressed via #9047

mjsax · 2020-07-23T04:10:23Z

    private final int retries;
    private final long retryBackOffMs;

+    @SuppressWarnings("deprecation") // TODO: remove in follow up PR when `RETRIES` is removed


Addressed via #9060

mjsax · 2020-07-23T04:18:17Z

        assertEquals(100, returnedProps.get(StreamsConfig.topicPrefix(TopicConfig.SEGMENT_BYTES_CONFIG)));
    }

+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Addressed via #9060

mjsax · 2020-07-23T04:19:26Z

    private MockAdminClient mockAdminClient;
    private InternalTopicManager internalTopicManager;

+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Addressed via #9060

Fixes flakiness in `KafkaAdminClientTest` as a result of #8864. Addresses the following flaky tests: - testAlterReplicaLogDirsPartialFailure - testDescribeLogDirsPartialFailure - testMetadataRetries Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jason Gustafson <jason@confluent.io>

mjsax · 2020-09-24T00:47:35Z


    private static final String SINK_TOPIC = "streamsResilienceSink";

+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Reverted via #9333

mjsax · 2020-09-24T00:47:40Z

        });
    }

+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Reverted via #9333

mjsax · 2020-09-24T00:47:50Z

 public class StreamsOptimizedTest {

-
+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Reverted via #9333

mjsax · 2020-09-24T00:47:58Z


 public class StreamsStandByReplicaTest {

+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Reverted via #9333

mjsax · 2020-09-24T00:48:04Z

        streams.close(Duration.ofSeconds(10));
    }

+    @SuppressWarnings("deprecation") // TODO revisit in follow up PR


Reverted via #9333

Fixes flakiness in `KafkaAdminClientTest` as a result of apache#8864. Addresses the following flaky tests: - testAlterReplicaLogDirsPartialFailure - testDescribeLogDirsPartialFailure - testMetadataRetries Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jason Gustafson <jason@confluent.io>

mjsax added streams producer kip Requires or implements a KIP labels Jun 13, 2020

mjsax commented Jun 13, 2020

View reviewed changes

mjsax force-pushed the kafka-9274-kip-572 branch from aa74cfa to 7d82538 Compare June 13, 2020 00:18

mjsax changed the title ~~KAFKA-9274: Mark retries config as deprecated~~ KAFKA-9274: Mark retries config as deprecated and add new task.timeout.ms config Jun 13, 2020

abbccdda reviewed Jun 13, 2020

View reviewed changes

hachikuji reviewed Jul 6, 2020

View reviewed changes

vvcephei reviewed Jul 6, 2020

View reviewed changes

mjsax added 2 commits July 20, 2020 13:01

KAFKA-9274: Mark retries config as deprecated

9e372d7

Github comments

e6c10d8

mjsax force-pushed the kafka-9274-kip-572 branch from b58d7a1 to e6c10d8 Compare July 21, 2020 00:12

mjsax mentioned this pull request Jul 21, 2020

KAFKA-9274: Remove retries for global task #9047

Merged

vvcephei reviewed Jul 21, 2020

View reviewed changes

mjsax merged commit 194c56f into apache:trunk Jul 21, 2020

mjsax deleted the kafka-9274-kip-572 branch July 21, 2020 19:34

mjsax commented Jul 21, 2020

View reviewed changes

mjsax commented Jul 23, 2020

View reviewed changes

mjsax mentioned this pull request Jul 23, 2020

KAFKA-9274: Remove retries from InternalTopicManager #9060

Merged

dajac mentioned this pull request Jul 28, 2020

MINOR; Make KafkaAdminClientTest.testDescribeLogDirsPartialFailure and KafkaAdminClientTest.testAlterReplicaLogDirsPartialFailure test more reliable #9091

Merged

3 tasks

mjsax mentioned this pull request Sep 24, 2020

KAFKA-9274: Revert deprecation of retries for producer and admin clients #9333

Merged

mjsax commented Sep 24, 2020

View reviewed changes


		private static final String SINK_TOPIC = "streamsResilienceSink";

		@SuppressWarnings("deprecation") // TODO revisit in follow up PR

		public class StreamsOptimizedTest {


		@SuppressWarnings("deprecation") // TODO revisit in follow up PR


		public class StreamsStandByReplicaTest {

		@SuppressWarnings("deprecation") // TODO revisit in follow up PR

Conversation

mjsax commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abbccdda left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jul 21, 2020

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mjsax commented Jun 13, 2020 •

edited

Loading

hachikuji left a comment •

edited

Loading