Skip to content

KAFKA-17497: Add e2e for zk migration with old controller#17131

Merged
chia7712 merged 5 commits intoapache:3.9from
frankvicky:KAFKA-17497
Sep 10, 2024
Merged

KAFKA-17497: Add e2e for zk migration with old controller#17131
chia7712 merged 5 commits intoapache:3.9from
frankvicky:KAFKA-17497

Conversation

@frankvicky
Copy link
Copy Markdown
Contributor

@frankvicky frankvicky commented Sep 9, 2024

JIRA: KAFKA-17497

This is follow-up of KAFKA-17011 that zk migration will encounter min=0 issue when the cluster consists of "new" broker and "old" controller.
The e2e zookeeper_migration_test [0] is a good test for online migration, but it does not test hybrid version. Hence, we can leverage it to increase test coverage.
[0] https://github.com/apache/kafka/blob/trunk/tests/kafkatest/tests/core/zookeeper_migration_test.py#L91

Committer Checklist (excluded from commit message)

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

@frankvicky frankvicky marked this pull request as draft September 9, 2024 04:48
chia7712
chia7712 reviewed Sep 9, 2024
View reviewed changes
Comment thread tests/kafkatest/tests/core/zookeeper_migration_test.py
def test_online_migration(self, roll_controller, from_kafka_version):
zk_quorum = partial(ServiceQuorumInfo, zk)
self.zk = ZookeeperService(self.test_context, num_nodes=1, version=DEV_BRANCH)
self.kafka = KafkaService(self.test_context,
Copy link
Copy Markdown
Member

@chia7712 chia7712 Sep 9, 2024

Choose a reason for hiding this comment

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

Please set IBP if the version is NOT DEV_BRANCH

@frankvicky frankvicky marked this pull request as ready for review September 9, 2024 06:54
chia7712
chia7712 reviewed Sep 9, 2024
View reviewed changes
Comment thread tests/kafkatest/tests/core/zookeeper_migration_test.py Outdated
server_prop_overrides = [["zookeeper.metadata.migration.enable", "false"]]

if from_kafka_version != str(DEV_BRANCH):
server_prop_overrides.append([config_property.INTER_BROKER_PROTOCOL_VERSION, "3.7"])
Copy link
Copy Markdown
Member

@chia7712 chia7712 Sep 9, 2024

Choose a reason for hiding this comment

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

Please use str(from_kafka_version

Copy link
Copy Markdown
Member

@chia7712 chia7712 Sep 9, 2024

Choose a reason for hiding this comment

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

My point was - please use str(from_kafka_version) to replace 3.7

mumrah
mumrah reviewed Sep 9, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mumrah mumrah 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 this test @frankvicky 👍

Is the purpose of the new test case to verify that the migration can happen on the latest software version with an older MV/IBP? E.g., 3.7 MV on 3.9 code base

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Sep 9, 2024

Is the purpose of the new test case to verify that the migration can happen on the latest software version with an older MV/IBP? E.g., 3.7 MV on 3.9 code base

The purpose is to verify the migration can happen on the latest broker with "older quorum controller". There was a min=0 issue in migrating (#16420 (comment)), and we did not add a e2e for the scenario before.

for another: the motivation of this jira/PR is from this comment (#16421 (comment)) that I realize the scenario is not easy to understand. Having a e2e case can help us to discuss the migration case

@frankvicky
Copy link
Copy Markdown
Contributor Author

frankvicky commented Sep 10, 2024

Hi @mumrah,

As @chia7712 mentioned, this test aims to ensure that the controller can work with brokers of higher versions. We need to verify that the higher version brokers can set the minimum supported version to 1 and communicate with the controller normally, as discussed in this #16421 (comment)

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Sep 10, 2024

We need to verify that the higher version brokers can set the minimum supported version to 1

This description is correct before #17128 gets merged. The new approach (#17128) will skip the features instead of changing the min version. The new approach can fix the issue of min=0 check in 3.8 controller
https://github.com/apache/kafka/blob/3.8/metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java#L470

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Sep 10, 2024

I'm going to merge this PR. The e2e case of 3.8 will be addressed by #17128

@chia7712 chia7712 merged commit 4b6437e into apache:3.9 Sep 10, 2024
chia7712 pushed a commit that referenced this pull request Sep 10, 2024
@frankvicky @chia7712
KAFKA-17497 Add e2e for zk migration with old controller (#17131)
2dc3ee0 
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
mingyen066 pushed a commit to mingyen066/kafka that referenced this pull request Sep 10, 2024
@frankvicky @mingyen066
KAFKA-17497 Add e2e for zk migration with old controller (apache#17131)
c4545ed 
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
@frankvicky @tedyu
KAFKA-17497 Add e2e for zk migration with old controller (apache#17131)
ea5fc37 
Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mumrah mumrah mumrah left review comments

@chia7712 chia7712 chia7712 left review comments

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

@frankvicky @chia7712 @mumrah