Skip to content

KAFKA-15468 [1/2]: Prevent transaction coordinator reloads on already loaded leaders#15139

Merged
jolshan merged 10 commits intoapache:trunkfrom
jolshan:kafka-15468-1
Jan 23, 2024
Merged

KAFKA-15468 [1/2]: Prevent transaction coordinator reloads on already loaded leaders#15139
jolshan merged 10 commits intoapache:trunkfrom
jolshan:kafka-15468-1

Conversation

@jolshan
Copy link
Copy Markdown
Member

@jolshan jolshan commented Jan 6, 2024

This originally was #14489 which covered 2 aspects -- reloading on partition epoch changes where leader epoch did not change and reloading when leader epoch changed but we were already the leader.

I've cut out the second part of the change since the first part is much simpler.

Redefining the TopicDelta fields to better distinguish when a leader is elected (leader epoch bump) vs when a leader has isr/replica changes (partition epoch bump). There are some cases where we bump the partition epoch but not the leader epoch. We do not need to do operations that only care about the leader epoch bump. (ie -- onElect callbacks)

Comment thread metadata/src/main/java/org/apache/kafka/image/LocalReplicaChanges.java Outdated
Copy link
Copy Markdown
Contributor

@artemlivshits artemlivshits left a comment

Choose a reason for hiding this comment

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

LGTM

@jsancio jsancio self-assigned this Jan 10, 2024
Copy link
Copy Markdown
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

Thanks. One minor suggestion.

Comment thread checkstyle/suppressions.xml Outdated
files="(QuorumController).java"/>
<suppress checks="(CyclomaticComplexity|NPathComplexity)"
files="(PartitionRegistration|PartitionChangeBuilder).java"/>
files="(PartitionRegistration|PartitionChangeBuilder|TopicDelta).java"/>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you try changing the code so that it doesn't warn you of path complexity? If it is not possible can you use annotations instead? I added annotation support a while back. Checkout: https://lists.apache.org/thread/mjxfofn823o1ozrzo36ogj17xmlrxod9

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In between when I wrote this originally and now a lot of JBOD code was added. I don't think I have the skills to make it simpler. I don't think there is a way to change my code. I can use the annotation if you think that keeps the suppression more contained

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes. Let's use annotation so that it only applies to the methods that violates our coding style.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've updated the PR

Copy link
Copy Markdown
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to merge if the tests are green.

@jolshan jolshan merged commit e00d36b into apache:trunk Jan 23, 2024
yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
… loaded leaders (apache#15139)

This originally was apache#14489 which covered 2 aspects -- reloading on partition epoch changes where leader epoch did not change and reloading when leader epoch changed but we were already the leader.

I've cut out the second part of the change since the first part is much simpler.

Redefining the TopicDelta fields to better distinguish when a leader is elected (leader epoch bump) vs when a leader has isr/replica changes (partition epoch bump). There are some cases where we bump the partition epoch but not the leader epoch. We do not need to do operations that only care about the leader epoch bump. (ie -- onElect callbacks)

Reviewers: Artem Livshits <alivshits@confluent.io>, José Armando García Sancio <jsancio@apache.org>
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
… loaded leaders (apache#15139)

This originally was apache#14489 which covered 2 aspects -- reloading on partition epoch changes where leader epoch did not change and reloading when leader epoch changed but we were already the leader.

I've cut out the second part of the change since the first part is much simpler.

Redefining the TopicDelta fields to better distinguish when a leader is elected (leader epoch bump) vs when a leader has isr/replica changes (partition epoch bump). There are some cases where we bump the partition epoch but not the leader epoch. We do not need to do operations that only care about the leader epoch bump. (ie -- onElect callbacks)

Reviewers: Artem Livshits <alivshits@confluent.io>, José Armando García Sancio <jsancio@apache.org>
Phuc-Hong-Tran pushed a commit to Phuc-Hong-Tran/kafka that referenced this pull request Jun 6, 2024
… loaded leaders (apache#15139)

This originally was apache#14489 which covered 2 aspects -- reloading on partition epoch changes where leader epoch did not change and reloading when leader epoch changed but we were already the leader.

I've cut out the second part of the change since the first part is much simpler.

Redefining the TopicDelta fields to better distinguish when a leader is elected (leader epoch bump) vs when a leader has isr/replica changes (partition epoch bump). There are some cases where we bump the partition epoch but not the leader epoch. We do not need to do operations that only care about the leader epoch bump. (ie -- onElect callbacks)

Reviewers: Artem Livshits <alivshits@confluent.io>, José Armando García Sancio <jsancio@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants