KAFKA-13488: Producer fails to recover if topic gets deleted midway by prat0318 · Pull Request #11552 · apache/kafka

prat0318 · 2021-11-30T07:15:12Z

Allow LeaderEpoch to be re-assigned to the new value from the Metadata Response if oldTopicId is not present in the cache. This is needed because oldTopicId is removed from the cache if the topic gets deleted but the LeaderEpoch is not removed. Hence, metadata for the newly recreated topic won't be accepted unless we allow oldTopicId to be null.

This is a fix on top of earlier made #10952 and #11004 PRs but still don't solve the bug mentioned in KAFKA-13488. This is now fixed in this PR.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

hachikuji

Thanks for the patch! I left a few comments for consideration.

hachikuji · 2021-12-01T01:59:23Z

            Integer currentEpoch = lastSeenLeaderEpochs.get(tp);
-            if (topicId != null && oldTopicId != null && !topicId.equals(oldTopicId)) {
+            // oldTopicId can be null (when metadata is fetched during topic recreation), update the metadata in that case as well.
+            if (topicId != null && !topicId.equals(oldTopicId)) {


I think this approach is reasonable. The root of the problem is the loose binding between the leader epoch and the topicId. I'm still a bit tempted to take this a little further and only permit the epoch check when the topicIds are matching. This is a little difficult to do at the moment because the epoch may be learned in contexts where we have not yet exposed the topicId. Hopefully this can be tightened up in https://issues.apache.org/jira/browse/KAFKA-13447. For now, this workaround seems ok.

hachikuji · 2021-12-01T02:04:07Z

            int newEpoch = partitionMetadata.leaderEpoch.get();
            Integer currentEpoch = lastSeenLeaderEpochs.get(tp);
-            if (topicId != null && oldTopicId != null && !topicId.equals(oldTopicId)) {
+            // oldTopicId can be null (when metadata is fetched during topic recreation), update the metadata in that case as well.


How about this?

// Between the time that a topic is deleted and re-created, the client may lose // track of the corresponding topicId (i.e. `oldTopicid` will be null). In this case, // when we discover the new topicId, we allow the corresponding leader epoch // to override the last seen value.

Also, would it make sense to move this into the corresponding branch that it applies to?

Ack. Changed the comment as per the suggestion.

would it make sense to move this into the corresponding branch that it applies to?

Sorry, couldn't get it. Can you elaborate on this please. (Do you mean a separate if branch? The current If branch deals with separate topicId, so that should be the one we should modify as part of this patch.)

hachikuji · 2021-12-01T02:11:14Z

-        // We should treat an added topic ID as though it is the same topic. Handle only when epoch increases.
-        // Don't update to an older one
-        metadataResponse = RequestTestUtils.metadataUpdateWithIds("dummy", 1, Collections.emptyMap(), Collections.singletonMap("topic-1", 1), _tp -> 1, topicIds);
+        // If the Older topic Id is null, we should go with the new TopicId as the leader Epoch


It might be worthwhile having a separate case which goes through the sequence described in the jira. Basically this:

Receive metadata response with topicID A.

Receive metadata response with UNKNOWN_TOPIC error.

Receive metadata response with topicID B.

added a new testcase with the suggested flow. Old test-case still needs to be changed as that case fails now, so modified the case as per the new changes.

nit: Older -> older; topic Id -> topic ID; TopicId -> topic ID; Epoch -> epoch.

prat0318 · 2021-12-03T01:31:38Z

@hachikuji Made the suggested changes. Please review.

prat0318 · 2021-12-06T16:09:16Z

@hachikuji Gentle bump on the review. PTAL.

prat0318 · 2021-12-13T14:55:36Z

@hachikuji @jolshan Bump on the review.

dajac

@prat0318 Thanks for the PR! I do agree with @hachikuji that the workaround looks reasonable. I have left some comments below (mainly nits).

dajac · 2021-12-15T16:07:33Z

+  * See the License for the specific language governing permissions and
+  * limitations under the License.
+  */
+


nit: Empty line could be removed.

dajac · 2021-12-16T09:19:44Z

+        MetadataResponse metadataResponse = emptyMetadataResponse();
+        metadata.updateWithCurrentRequestVersion(metadataResponse, false, 0L);
+
+        Map<String, Uuid> topicIds = Collections.singletonMap("topic-1", Uuid.randomUuid());


nit: We could have kept the comment before the block to be consistent with the other two blocks. I just wanted to rework it a bit.

dajac · 2021-12-16T09:20:23Z

+        metadata.updateWithCurrentRequestVersion(metadataResponse, false, 1L);
+        assertEquals(Optional.of(10), metadata.lastSeenLeaderEpoch(tp));
+
+        // Create topic-1 again but this time with a topic ID bar. LeaderEpoch should be updated to new even if lower.


nit: Create topic-1 again but this time with a topic ID.?

dajac · 2021-12-16T09:29:36Z

+            new TopicPartition(topic, 1) -> Seq(1, 0)
+        )
+
+        // Change leader to 1 for both the partitions to increase leader Epoch from 0 -> 1


nit: Epoch -> epoch.

dajac · 2021-12-16T09:30:24Z

@prat0318 Thanks for the update. I left a few more nits to fix typos. Would you have time to quickly address them?

prat0318 · 2021-12-16T09:33:08Z

@prat0318 Thanks for the update. I left a few more nits to fix typos. Would you have time to quickly address them?

@dajac Thanks again for the review. I have addressed the suggestions. Please re-review.

dajac

LGTM, thanks for the patch!

…11552) Allow the leader epoch to be re-assigned to the new value from the Metadata response if `oldTopicId` is not present in the cache. This is needed because `oldTopicId` is removed from the cache if the topic gets deleted but the leader epoch is not removed. Hence, metadata for the newly recreated topic won't be accepted unless we allow `oldTopicId` to be null. Reviewers: Jason Gustafson <jason@confluent.io>, David Jacot <djacot@confluent.io>

dajac · 2021-12-16T15:16:14Z

Merged to trunk, 3.1, 3.0 and 2.8.

…pache#11552) Allow the leader epoch to be re-assigned to the new value from the Metadata response if `oldTopicId` is not present in the cache. This is needed because `oldTopicId` is removed from the cache if the topic gets deleted but the leader epoch is not removed. Hence, metadata for the newly recreated topic won't be accepted unless we allow `oldTopicId` to be null. Reviewers: Jason Gustafson <jason@confluent.io>, David Jacot <djacot@confluent.io>

KAFKA-13488: Producer fails to recover if topic gets deleted midway

a33c7da

hachikuji reviewed Dec 1, 2021

View reviewed changes

Add review suggestions

e15d928

prat0318 requested a review from hachikuji December 1, 2021 06:19

dajac reviewed Dec 15, 2021

View reviewed changes

prat0318 added 2 commits December 16, 2021 00:13

Add Review suggestions

5d83e4d

Capitalize start of the comment lines

0675c57

prat0318 requested a review from dajac December 15, 2021 18:57

Fix build for Scala 12

d4da13c

dajac reviewed Dec 16, 2021

View reviewed changes

Add review suggestions

df4630e

prat0318 requested a review from dajac December 16, 2021 09:32

dajac approved these changes Dec 16, 2021

View reviewed changes

dajac merged commit f653cb7 into apache:trunk Dec 16, 2021

fabien-chebel mentioned this pull request Aug 19, 2022

MINOR: reduce Metadata log volume when last seen epoch is not updated #12541

Closed

3 tasks

Conversation

prat0318 commented Nov 30, 2021

Committer Checklist (excluded from commit message)

Uh oh!

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

prat0318 Dec 1, 2021 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac Dec 16, 2021 • 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!

prat0318 commented Dec 3, 2021

Uh oh!

prat0318 commented Dec 6, 2021

Uh oh!

prat0318 commented Dec 13, 2021

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

dajac commented Dec 16, 2021

Uh oh!

prat0318 commented Dec 16, 2021

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

dajac commented Dec 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prat0318 Dec 1, 2021 •

edited

Loading

dajac Dec 16, 2021 •

edited

Loading