MINOR: Remove dead code maybeWarnIfOversizedRecords
#19316
Conversation
The `metadataVersionSupplier` is unused after this - remove it.
| @@ -161,15 +157,6 @@ class ReplicaFetcherThread(name: String, | |||
| } | |||
| } | |||
|
|
|||
| private def maybeWarnIfOversizedRecords(records: MemoryRecords, topicPartition: TopicPartition): Unit = { | |||
| // oversized messages don't cause replication to fail from fetch request version 3 (KIP-74) | |||
| if (metadataVersionSupplier().fetchRequestVersion <= 2 && records.sizeInBytes > 0 && records.validBytes <= 0) | |||
There was a problem hiding this comment.
It seems another use case of fetchRequestVersion is dead code. Maybe we can remove it as well?
val version: Short = if (metadataVersion.fetchRequestVersion >= 13 && !fetchData.canUseTopicIds) {
12
} else {
metadataVersion.fetchRequestVersion
}There was a problem hiding this comment.
I think you're saying that IBP 3.3 implies fetch request 13 or higher. That makes sense, I pushed a commit that removes that check.
There was a problem hiding this comment.
I also updated the PR description.
| @@ -203,7 +203,7 @@ class RemoteLeaderEndPoint(logPrefix: String, | |||
| None | |||
| } else { | |||
| val metadataVersion = metadataVersionSupplier() | |||
| val version: Short = if (metadataVersion.fetchRequestVersion >= 13 && !fetchData.canUseTopicIds) { | |||
| val version: Short = if (!fetchData.canUseTopicIds) { | |||
There was a problem hiding this comment.
This is interesting. IIRC, the topic id of fetch is introduced in 3.1, so canUseTopicIds should be always enabled after we bump the baseline of MV to 3.3.
There was a problem hiding this comment.
Yes, I looked into that after your original comment, but it's a larger change since some of the code is shared and topic ids are optional in the shared code. I think we can file a JIRA ticket to make this change via a separate PR.
There was a problem hiding this comment.
You are right. file https://issues.apache.org/jira/browse/KAFKA-19065
There was a problem hiding this comment.
As part of that ticket, we should also remove the getOrElse below and change PartitionFetchState.topicId not to be Optional, etc.
fetchState.topicId.getOrElse(Uuid.ZERO_UUID),
The `metadataVersionSupplier` is unused after this - remove it. Also remove redundant `metadataVersion.fetchRequestVersion >= 13` check in `RemoteLeaderEndPoint` - the minimum version returned by this method is `13`. Reviewers: Chia-Ping Tsai <[email protected]>
The
metadataVersionSupplieris unused after this - remove it.Also remove redundant
metadataVersion.fetchRequestVersion >= 13check inRemoteLeaderEndPoint- the minimum version returned by this method is13.Reviewers: Chia-Ping Tsai [email protected]