Skip to content

KAFKA-13990: KRaft controller should return right features in ApiVersionResponse#12294

Merged
cmccabe merged 1 commit intoapache:trunkfrom
dengziming:KAFKA-13990
Aug 31, 2022
Merged

KAFKA-13990: KRaft controller should return right features in ApiVersionResponse#12294
cmccabe merged 1 commit intoapache:trunkfrom
dengziming:KAFKA-13990

Conversation

@dengziming
Copy link
Copy Markdown
Member

@dengziming dengziming commented Jun 14, 2022

More detailed description of your change

Updating metadata.version using AdminClient will fail with the following exception:

java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.InvalidUpdateVersionException: Invalid update version 7 for feature metadata.version. Controller 3000 does not support this feature.

This is because we just return empty supported features in Controller ApiVersionResponse, so QuorumFeatures.reasonNotSupported will always assume we don't support this version.

Summary of testing strategy (including rationale)
KRaftClusterTest.testUpdateMetadataVersion

Committer Checklist (excluded from commit message)

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

@dengziming
Copy link
Copy Markdown
Member Author

Hello @cmccabe , in #12207 you moved the validation from FeatureControlManager.replay to FeatureControlManager.updateFeature, but it's still failing since controller doesn't return right supported versions in ApiVersionResponse.

@dengziming
Copy link
Copy Markdown
Member Author

This is an issue of big influence since it will prevent us from upgrading KRaft cluster, also ping @hachikuji @mumrah

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.

This test case will fail without this change, can you take a look at this PR @jsancio , I wish this can be merged into 3.3.

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.

Thanks. I'll mark as a blocker for now but it would be good for @mumrah or @cmccabe to comment and review.

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Aug 31, 2022

Thanks for finding this, @dengziming . It is a great find and definitely a 3.3 blocker.

Copy link
Copy Markdown
Contributor

@cmccabe cmccabe left a comment

Choose a reason for hiding this comment

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

LGTM

@cmccabe cmccabe merged commit 9e71d81 into apache:trunk Aug 31, 2022
@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Aug 31, 2022

So, the reason we didn't notice this bug earlier is that it doesn't affect single-node clusters. And in the multi-node ducktape test that I wrote, the upgrade command was failing but we didn't notice because the error code returned was 0 (success). I will open a PR to fix the kafka-features.sh return code.

cmccabe pushed a commit that referenced this pull request Aug 31, 2022
…ionResponse (#12294)

Previously, the KRaft controller was incorrectly reporting an empty feature set in
ApiVersionResponse. This was preventing any multi-node clusters from being upgraded via
kafka-features.sh, since they would incorrectly believe that metadata.version was not a supported
feature. This PR adds a regression test for this bug, KRaftClusterTest.testUpdateMetadataVersion.

Reviewers: José Armando García Sancio <jsancio@gmail.com>, Colin P. McCabe <cmccabe@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Kafka Broker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants