KAFKA-9307: Make transaction metadata loading resilient to previous errors by dhruvilshah3 · Pull Request #7840 · apache/kafka

dhruvilshah3 · 2019-12-17T06:34:10Z

Allow transaction metadata to be reloaded, even if it already exists as of a previous epoch. This helps with cases where a previous become-follower transition failed to unload corresponding metadata.

…rrors

junrao

@dhruvilshah3 : Thanks for the PR. LGTM. Just a few minor comments below. @hachikuji , do you want to take a look at the PR too?

junrao · 2019-12-18T19:19:43Z

-      throw new IllegalStateException(errorMsg)
-    }
+    if (currentTxnMetadataCacheEntry.isDefined)
+      info(s"Unloaded $currentTxnMetadataCacheEntry for $txnTopicPartition as part of loading metadata at epoch $coordinatorEpoch")


It seems that we need to log currentTxnMetadataCacheEntry.get ?

junrao · 2019-12-18T19:21:14Z

+private[transaction] case class TxnMetadataCacheEntry(coordinatorEpoch: Int,
+                                                      metadataPerTransactionalId: Pool[String, TransactionMetadata]) {
+  override def toString: String = {
+    s"TxnMetadataCacheEntry(coordinatorEpoch=$coordinatorEpoch, cacheSize=${metadataPerTransactionalId.size})"


cacheSize => transactionalEntries?

junrao · 2019-12-18T19:23:39Z

            case None =>
-              info(s"Trying to remove cached transaction metadata for $topicPartition on follower transition but there is no entries remaining; " +
-                s"it is likely that another process for removing the cached entries has just executed earlier before")
+              info(s"No cached metadata found for $topicPartition during become-follower transition")


It's useful to mention the cached metadata is for transactions.

dhruvilshah3 · 2019-12-19T00:34:26Z

+    // The operations performed during immigration must be resilient to any previous errors we saw or partial state we
+    // left off during the unloading phase. Ensure we remove all associated state for this partition before we continue
+    // loading it.
+    completeEmigration(txnTopicPartitionId)


@junrao I added this in after discussing with @hachikuji. Let me know what you think.

junrao

@dhruvilshah3 : Thanks for the updated PR. The patch itself seems fine. However, I am not it completely fixes the issue in the jira since validateTransactionTopicPartitionCountIsStable() is called when loading txns and the same ZK exception could be hit then.

junrao · 2019-12-19T02:48:50Z

+
+    // first call must be to remove transaction markers, and then we start loading transactions
+    inOrderVerifier.verify(transactionMarkerChannelManager).removeMarkersForTxnTopicPartition(txnTopicPartitionId)
+    inOrderVerifier.verify(transactionManager).loadTransactionsForTxnTopicPartition(ArgumentMatchers.eq(txnTopicPartitionId),


Hmm, is there any value in this test? We are just verifying ordering of 2 methods in the code.

dhruvilshah3 · 2019-12-19T15:52:43Z

@junrao the idea is that if the leader's session has expired then transaction metadata loading would fail. However, after this point, we would expect the broker to again be made the leader in the future after it has re-established its session, and metadata loading would then succeed. Let me know if this makes sense.

junrao · 2019-12-19T16:48:55Z

@dhruvilshah3 : That's in general true. However, there is a small window from the time a leader's ZK session is expired to the time when a new leader is elected. In that window, client requests will still be routed to the current leader. If the leader accepts/rejects a txn request based on inaccurate txn cache, it could have other implications.

hachikuji

Thanks, left a few small comments.

hachikuji · 2019-12-19T19:27:38Z

+    completeEmigration(txnTopicPartitionId)
+  }
+
+  private def completeEmigration(txnTopicPartitionId: Int): Unit = {


nit: this function doesn't seem to add much value

hachikuji · 2019-12-19T19:28:57Z

-      throw new IllegalStateException(errorMsg)
-    }
+    if (currentTxnMetadataCacheEntry.isDefined)
+      info(s"Unloaded transaction metadata ${currentTxnMetadataCacheEntry.get} from $txnTopicPartition as part of " +


Considering whether it is worth making this a warn.. I think it is still an unexpected state in the system.

hachikuji · 2019-12-19T19:31:31Z

-      fatal(errorMsg)
-      throw new IllegalStateException(errorMsg)
-    }
+    if (currentTxnMetadataCacheEntry.isDefined)


nit: we can make this a foreach

hachikuji · 2019-12-19T19:32:13Z

        }
      }
+
+      info(s"Completed loading transaciton metadata from $topicPartition at epoch $coordinatorEpoch")


nit: "at epoch" -> "for coordinator epoch"?

Also, typo: transaciton

- Remove ZK lookup when unloading metadata

hachikuji

LGTM

junrao

@dhruvilshah3 : Thanks for the PR. The approach looks good. Just one comment below.

junrao · 2019-12-20T19:11:22Z

+    assertEquals(0, transactionManager.loadingPartitions.size)
+    assertEquals(0, transactionManager.leavingPartitions.size)
+
+    // simulate a failed emigration; immigrate partition at epoch 1


Hmm, where is the logic for failing an emigration?

I'll reword the comment. The test is now just checking if the partition can be reimmigrated even though we didn't get to execute the emigration.

dhruvilshah3 · 2019-12-23T07:29:52Z

Retest this please

hachikuji · 2019-12-23T16:29:35Z

retest this please

hachikuji · 2019-12-23T17:47:59Z

retest this please

hachikuji · 2019-12-23T19:08:43Z

retest this please

dhruvilshah3 · 2019-12-23T20:09:28Z

Retest this please

addisonhuddy · 2019-12-23T20:55:48Z

Can someone confirm which minor branches this fix will be back-ported to?

hachikuji · 2019-12-23T23:21:07Z

@addisonhuddy I will merge back to 2.2 at least.

…rrors (#7840) Allow transaction metadata to be reloaded, even if it already exists as of a previous epoch. This helps with cases where a previous become-follower transition failed to unload corresponding metadata. Reviewers: Jun Rao <junrao@gmail.com>, Jason Gustafson <jason@confluent.io>

…rrors (apache#7840) Allow transaction metadata to be reloaded, even if it already exists as of a previous epoch. This helps with cases where a previous become-follower transition failed to unload corresponding metadata. Reviewers: Jun Rao <junrao@gmail.com>, Jason Gustafson <jason@confluent.io>

KAFKA-9307: Make transaction metadata loading resilient to previous e…

df8687f

…rrors

junrao approved these changes Dec 18, 2019

View reviewed changes

dhruvilshah3 added 2 commits December 18, 2019 11:53

Address review comments

9a28b70

Ensure txn markers are removed before starting to load txn partition

6534bd7

dhruvilshah3 commented Dec 19, 2019

View reviewed changes

junrao reviewed Dec 19, 2019

View reviewed changes

hachikuji reviewed Dec 19, 2019

View reviewed changes

dhruvilshah3 added 2 commits December 19, 2019 22:32

- Address review comments

865f5f8

- Remove ZK lookup when unloading metadata

Add test

49cafdd

hachikuji approved these changes Dec 20, 2019

View reviewed changes

junrao reviewed Dec 20, 2019

View reviewed changes

Fix comment

7d6ebb0

dhruvilshah3 closed this Dec 20, 2019

dhruvilshah3 reopened this Dec 20, 2019

hachikuji merged commit 0f0eb49 into apache:trunk Dec 23, 2019

dhruvilshah3 deleted the kafka-9307 branch December 23, 2019 23:21

Uh oh!

Conversation

dhruvilshah3 commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvilshah3 commented Dec 19, 2019

Uh oh!

junrao commented Dec 19, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

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

dhruvilshah3 commented Dec 23, 2019

Uh oh!

hachikuji commented Dec 23, 2019

Uh oh!

hachikuji commented Dec 23, 2019

Uh oh!

hachikuji commented Dec 23, 2019

Uh oh!

dhruvilshah3 commented Dec 23, 2019

Uh oh!

addisonhuddy commented Dec 23, 2019

Uh oh!

hachikuji commented Dec 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dhruvilshah3 commented Dec 17, 2019 •

edited

Loading