-
Notifications
You must be signed in to change notification settings - Fork 15.4k
MINOR: Handle lastFetchedEpoch/divergingEpoch in FetchSession and DelayedFetch #9434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,7 @@ class DelayedFetch(delayMs: Long, | |
| * Case E: This broker is the leader, but the requested epoch is now fenced | ||
| * Case F: The fetch offset locates not on the last segment of the log | ||
| * Case G: The accumulated bytes from all the fetching partitions exceeds the minimum bytes | ||
| * Case H: A diverging epoch was found, return response to trigger truncation | ||
| * Upon completion, should return whatever data is available for each valid partition | ||
| */ | ||
| override def tryComplete(): Boolean = { | ||
|
|
@@ -88,6 +89,7 @@ class DelayedFetch(delayMs: Long, | |
| try { | ||
| if (fetchOffset != LogOffsetMetadata.UnknownOffsetMetadata) { | ||
| val partition = replicaManager.getPartitionOrException(topicPartition) | ||
|
|
||
| val offsetSnapshot = partition.fetchOffsetSnapshot(fetchLeaderEpoch, fetchMetadata.fetchOnlyLeader) | ||
|
|
||
| val endOffset = fetchMetadata.fetchIsolation match { | ||
|
|
@@ -96,6 +98,7 @@ class DelayedFetch(delayMs: Long, | |
| case FetchTxnCommitted => offsetSnapshot.lastStableOffset | ||
| } | ||
|
|
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: unneeded newline |
||
| // Go directly to the check for Case G if the message offsets are the same. If the log segment | ||
| // has just rolled, then the high watermark offset will remain the same but be on the old segment, | ||
| // which would incorrectly be seen as an instance of Case F. | ||
|
|
@@ -118,6 +121,14 @@ class DelayedFetch(delayMs: Long, | |
| accumulatedSize += bytesAvailable | ||
| } | ||
| } | ||
|
|
||
| // Case H: If truncation has caused diverging epoch while this request was in purgatory, return to trigger truncation | ||
| fetchStatus.fetchInfo.lastFetchedEpoch.ifPresent { fetchEpoch => | ||
| if (partition.hasDivergingEpoch(fetchLeaderEpoch, fetchEpoch, fetchStatus.fetchInfo.fetchOffset)) { | ||
| debug(s"Satisfying fetch $fetchMetadata since it has diverging epoch requiring truncation for partition $topicPartition.") | ||
| return forceComplete() | ||
| } | ||
| } | ||
| } | ||
| } catch { | ||
| case _: NotLeaderOrFollowerException => // Case A or Case B | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,8 @@ class CachedPartition(val topic: String, | |
| var highWatermark: Long, | ||
| var leaderEpoch: Optional[Integer], | ||
| var fetcherLogStartOffset: Long, | ||
| var localLogStartOffset: Long) | ||
| var localLogStartOffset: Long, | ||
| var lastFetchedEpoch: Optional[Integer] = Optional.empty[Integer]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to provide a default here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
| extends ImplicitLinkedHashCollection.Element { | ||
|
|
||
| var cachedNext: Int = ImplicitLinkedHashCollection.INVALID_INDEX | ||
|
|
@@ -96,21 +97,22 @@ class CachedPartition(val topic: String, | |
|
|
||
| def this(part: TopicPartition, reqData: FetchRequest.PartitionData) = | ||
| this(part.topic, part.partition, reqData.maxBytes, reqData.fetchOffset, -1, | ||
| reqData.currentLeaderEpoch, reqData.logStartOffset, -1) | ||
| reqData.currentLeaderEpoch, reqData.logStartOffset, -1, reqData.lastFetchedEpoch) | ||
|
|
||
| def this(part: TopicPartition, reqData: FetchRequest.PartitionData, | ||
| respData: FetchResponse.PartitionData[Records]) = | ||
| this(part.topic, part.partition, reqData.maxBytes, reqData.fetchOffset, respData.highWatermark, | ||
| reqData.currentLeaderEpoch, reqData.logStartOffset, respData.logStartOffset) | ||
| reqData.currentLeaderEpoch, reqData.logStartOffset, respData.logStartOffset, reqData.lastFetchedEpoch) | ||
|
|
||
| def reqData = new FetchRequest.PartitionData(fetchOffset, fetcherLogStartOffset, maxBytes, leaderEpoch) | ||
| def reqData = new FetchRequest.PartitionData(fetchOffset, fetcherLogStartOffset, maxBytes, leaderEpoch, lastFetchedEpoch) | ||
|
|
||
| def updateRequestParams(reqData: FetchRequest.PartitionData): Unit = { | ||
| // Update our cached request parameters. | ||
| maxBytes = reqData.maxBytes | ||
| fetchOffset = reqData.fetchOffset | ||
| fetcherLogStartOffset = reqData.logStartOffset | ||
| leaderEpoch = reqData.currentLeaderEpoch | ||
| lastFetchedEpoch = reqData.lastFetchedEpoch | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check the error? Or are you relying on the check below failing if
UNDEFINED_EPOCHis returned?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hachikuji Thanks for the review. I took the check from
Partition.readRecords, but we throw exceptions there to return appropriate errors. I was thinking we would return true here for undefined epochs because of the check below and that would go through the other code path to return the appropriate errors or diverging epoch. Do you think we should do the same error handling hchecks ere as in readRecords to make the flow more obvious?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, moved the check to DelayedFetch.