Skip to content

KAFKA-10739; Replace EpochEndOffset with automated protocol#9630

Merged
dajac merged 14 commits intoapache:trunkfrom
dajac:KAFKA-10739
Dec 3, 2020
Merged

KAFKA-10739; Replace EpochEndOffset with automated protocol#9630
dajac merged 14 commits intoapache:trunkfrom
dajac:KAFKA-10739

Conversation

@dajac
Copy link
Copy Markdown
Member

@dajac dajac commented Nov 20, 2020 •
edited
Loading

This patch follows up #9547. It refactors KafkaApis, ReplicaManager and Partition to use OffsetForLeaderPartitionResult instead of EpochEndOffset. In the mean time, it removes OffsetsForLeaderEpochRequest#epochsByTopicPartition and OffsetsForLeaderEpochResponse#responses and replaces their usages to use the automated protocol directly. Finally, it removes old constructors in OffsetsForLeaderEpochResponse. The patch relies on existing tests.

Committer Checklist (excluded from commit message)

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

@dajac dajac changed the title KAFKA-10739; WIP KAFKA-10739; Replace EpochEndOffset with automated protocol Nov 23, 2020
@dajac dajac marked this pull request as ready for review November 23, 2020 08:39
@dajac
Copy link
Copy Markdown
Member Author

dajac commented Nov 23, 2020

cc @hachikuji @chia7712

chia7712
chia7712 approved these changes Nov 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall +1. Three minor questions are left.

Also, there are a lot of data structure like Map[TopicPartition, OffsetForLeaderPartitionResult]. It contains duplicate (int) partition info. Not sure whether
it is worth replacing it by Map[String,List[OffsetForLeaderPartitionResult]] to eliminate duplicate partition info. It will bring a bunch of changes :(

Comment thread core/src/main/scala/kafka/server/ReplicaManager.scala Outdated
Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/epoch/util/ReplicaFetcherMockBlockingSend.scala Outdated
@dajac
Copy link
Copy Markdown
Member Author

dajac commented Nov 26, 2020

Also, there are a lot of data structure like Map[TopicPartition, OffsetForLeaderPartitionResult]. It contains duplicate (int) partition info. Not sure whether
it is worth replacing it by Map[String,List[OffsetForLeaderPartitionResult]] to eliminate duplicate partition info. It will bring a bunch of changes :(

Yeah, I considered this actually. Finally, I went with keeping TopicPartition as a key mainly because TopicPartition is required by most of the callers.

hachikuji
hachikuji reviewed Dec 1, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement. Left a couple small comments.

Comment thread core/src/main/scala/kafka/server/ReplicaManager.scala Outdated
Comment thread clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java Outdated
@dajac
Copy link
Copy Markdown
Member Author

dajac commented Dec 2, 2020

@hachikuji Thanks for your feedback. I have addressed your points.

@dajac dajac requested a review from hachikuji December 2, 2020 09:04
hachikuji
hachikuji approved these changes Dec 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. LGTM. Just a minor comment you can consider (no need for re-review).

Comment thread clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java Outdated
dajac added 14 commits December 3, 2020 11:14
@dajac
Copy link
Copy Markdown
Member Author

dajac commented Dec 3, 2020 •
edited
Loading

I just rebased the PR cause it had conflicts with 7ecc3a5.

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Dec 3, 2020

Test failure is unrelated:

  • Build / JDK 8 / kafka.api.TransactionsTest.testBumpTransactionalEpoch

@dajac dajac merged commit 10364e4 into apache:trunk Dec 3, 2020
@dajac dajac deleted the KAFKA-10739 branch December 3, 2020 17:50
ijuma added a commit to ijuma/kafka that referenced this pull request Dec 3, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into skip-to-struc…
f05f217 
…t-for-generated-requests

* apache-github/trunk:
MINOR: Fix flaky test shouldQueryOnlyActivePartitionStoresByDefault
(apache#9681)
  KAFKA-10799 AlterIsr utilizes ReplicaManager ISR metrics (apache#9677)
  MINOR: Fix KTable-KTable foreign-key join example (apache#9683)
KAFKA-10473: Add docs on partition size-on-disk, and other log-related
metrics (apache#9276)
  KAFKA-10739; Replace EpochEndOffset with automated protocol (apache#9630)
KAFKA-10460: ReplicaListValidator format checking is incomplete
(apache#9326)
KAFKA-10554; Perform follower truncation based on diverging epochs in
Fetch response (apache#9382)
  MINOR: Align the UID inside/outside container (apache#9652)
KAFKA-10794 Replica leader election is too slow in the case of too
many partitions (apache#9675)
KAFKA-10090 Misleading warnings: The configuration was supplied but i…
(apache#8826)

clients/src/main/java/org/apache/kafka/common/requests/OffsetsForLeaderEpochResponse.java
clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java
core/src/test/scala/unit/kafka/server/epoch/util/ReplicaFetcherMockBlockingSend.scala
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 approved these changes

@hachikuji hachikuji hachikuji approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dajac @chia7712 @hachikuji