Skip to content

KAFKA-18486: Remove ReplicaManager#becomeLeaderOrFollower #20037

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

Merged
merged 12 commits into from
Jun 29, 2025

Conversation

TaiJuWu
Copy link
Collaborator

@TaiJuWu TaiJuWu commented Jun 25, 2025

The PR do following:

  1. Remove ReplicaManager#becomeLeaderOrFollower.
  2. Remove LeaderAndIsrRequest and LeaderAndIsrResponse
  3. Migrate LeaderAndIsrRequest.PartitionState to server-common module
    and change to PartitionState
  4. Remove ControllerEpoch from PartitionState
  5. Remove isShuttingDown from BrokerServer and ReplicaManager

Reviewers: Kuan-Po Tseng brandboat@gmail.com, Chia-Ping Tsai
chia7712@gmail.com

@github-actions github-actions bot added triage PRs from the community core Kafka Broker labels Jun 25, 2025
Copy link
Member

@brandboat brandboat left a comment

Choose a reason for hiding this comment

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

In ReplicaManager, there is one unused variable
@volatile private[server] var controllerEpoch: Int = 0

And there are plenty of code in LeaderAndIsrRequest.java could be removed.

@github-actions github-actions bot removed the triage PRs from the community label Jun 26, 2025
Copy link
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.

@TaiJuWu thanks for you patch. alterPartitionManager is not used, so please remove it also.

byte leaderRecoveryState;

public PartitionState() {
this.topicName = "";
Copy link
Member

Choose a reason for hiding this comment

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

Could you please remove the default constructor by initializing these variables directly?

public class PartitionState {
    String topicName = "";
    int partitionIndex = 0;
    int leader = 0;

completeDelayedOperationsWhenNotPartitionLeader(partition.topicPartition, partition.topicId)
}

if (isShuttingDown.get()) {
Copy link
Member

Choose a reason for hiding this comment

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

isShuttingDown is useless now, so could you please remove it

@TaiJuWu
Copy link
Collaborator Author

TaiJuWu commented Jun 27, 2025

@TaiJuWu thanks for you patch. alterPartitionManager is not used, so please remove it also.

Thanks for review, we can't remove alterPartitionManager from ReplicaManager because we need to get it when creating partition.

alterIsrManager = replicaManager.alterPartitionManager)

@@ -354,7 +354,6 @@ class BrokerServer(
logDirFailureChannel = logDirFailureChannel,
alterPartitionManager = alterPartitionManager,
brokerTopicStats = brokerTopicStats,
isShuttingDown = isShuttingDown,
Copy link
Member

Choose a reason for hiding this comment

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

we could remove isShuttingDown from BrokerServer, right?

Copy link
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.

LGTM. we could have follow-ups for advanced cleanup in order to avoid pouring changes to this PR.

@chia7712
Copy link
Member

The follow-ups are listed below.

  1. https://issues.apache.org/jira/browse/KAFKA-19447
  2. remove FlattenedIterator

@chia7712 chia7712 merged commit bd14ed2 into apache:trunk Jun 29, 2025
23 checks passed
@TaiJuWu TaiJuWu deleted the KAFKA-18486_ branch June 30, 2025 01:42
chia7712 pushed a commit that referenced this pull request Jun 30, 2025
Remove FlattenedIterator. It’s no longer used anywhere after
#20037.

Reviewers: TengYao Chi <kitingiao@gmail.com>, Lan Ding
 <isDing_L@163.com>, Chia-Ping Tsai <chia7712@gmail.com>
jiafu1115 pushed a commit to jiafu1115/kafka that referenced this pull request Jul 2, 2025
The PR do following:

1. Remove  ReplicaManager#becomeLeaderOrFollower.
2. Remove `LeaderAndIsrRequest` and `LeaderAndIsrResponse`
3. Migrate `LeaderAndIsrRequest.PartitionState` to server-common module
and change to `PartitionState`
4. Remove `ControllerEpoch` from PartitionState
5. Remove `isShuttingDown` from BrokerServer and ReplicaManager

Reviewers: Kuan-Po Tseng <brandboat@gmail.com>, Chia-Ping Tsai
 <chia7712@gmail.com>
jiafu1115 pushed a commit to jiafu1115/kafka that referenced this pull request Jul 2, 2025
Remove FlattenedIterator. It’s no longer used anywhere after
apache#20037.

Reviewers: TengYao Chi <kitingiao@gmail.com>, Lan Ding
 <isDing_L@163.com>, Chia-Ping Tsai <chia7712@gmail.com>
jiafu1115 pushed a commit to jiafu1115/kafka that referenced this pull request Jul 3, 2025
The PR do following:

1. Remove  ReplicaManager#becomeLeaderOrFollower.
2. Remove `LeaderAndIsrRequest` and `LeaderAndIsrResponse`
3. Migrate `LeaderAndIsrRequest.PartitionState` to server-common module
and change to `PartitionState`
4. Remove `ControllerEpoch` from PartitionState
5. Remove `isShuttingDown` from BrokerServer and ReplicaManager

Reviewers: Kuan-Po Tseng <brandboat@gmail.com>, Chia-Ping Tsai
 <chia7712@gmail.com>
jiafu1115 pushed a commit to jiafu1115/kafka that referenced this pull request Jul 3, 2025
Remove FlattenedIterator. It’s no longer used anywhere after
apache#20037.

Reviewers: TengYao Chi <kitingiao@gmail.com>, Lan Ding
 <isDing_L@163.com>, Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants