Skip to content

KAFKA-13228: ApiVersionRequest is not properly handled in KRaft co-resident mode#11261

Closed
dengziming wants to merge 2 commits into
apache:trunkfrom
dengziming:KAFKA-13228-kraft-api-version
Closed

KAFKA-13228: ApiVersionRequest is not properly handled in KRaft co-resident mode#11261
dengziming wants to merge 2 commits into
apache:trunkfrom
dengziming:KAFKA-13228-kraft-api-version

Conversation

@dengziming

@dengziming dengziming commented Aug 26, 2021

Copy link
Copy Markdown
Member

More detailed description of your change

When I described quorum in Kraft mode I got org.apache.kafka.common.errors.UnsupportedVersionException: The broker does not support DESCRIBE_QUORUM.
This happens because we only concerns ApiKeys.zkBrokerApis() when we call NodeApiVersions.create(), we should use ApiKeys.controllerApiVersions when in Kraft mode.

Summary of testing strategy (including rationale)

After this change, the DESCRIBE_QUORUM request was property handled and got a correct response:

TopicData(topicName='__cluster_metadata', partitions=[PartitionData(partitionIndex=0, errorCode=0, leaderId=1, leaderEpoch=30, highWatermark=141, currentVoters=[ReplicaState(replicaId=1, logEndOffset=141)], observers=[])])

@dengziming
dengziming marked this pull request as draft August 26, 2021 13:41
@dengziming
dengziming force-pushed the KAFKA-13228-kraft-api-version branch 2 times, most recently from 631b0de to f38c1e1 Compare August 29, 2021 13:26
@dengziming
dengziming marked this pull request as ready for review August 29, 2021 14:54

@dengziming dengziming left a comment

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.

Hello @hachikuji , you changed NodeApiVersions.create() to account for ApiKeys.zkBrokerApis() in #10066 , but this is not right if the controller is KRaft mode, I added a controllerBrokerType in BrokerToControllerChannelManagerImpl to solve this problem. Also cc @abbccdda

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 will be solved in #11274

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.

These overrides are unnecessary if we use ClusterTestExtensions

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 will result in NPE

@dengziming dengziming changed the title KAFKA-13228: Ensure ApiVersionRequest is properly handled in KRaft KAFKA-13228: ApiVersionRequest is not properly handled in KRaft Oct 30, 2021
@dengziming
dengziming force-pushed the KAFKA-13228-kraft-api-version branch from f38c1e1 to 678ccca Compare October 30, 2021 10:53
@dengziming
dengziming force-pushed the KAFKA-13228-kraft-api-version branch from f6a73ea to ce1c2a0 Compare November 6, 2021 02:03
@dengziming

Copy link
Copy Markdown
Member Author

I also found that we do not support UpdateFeaturesRequest in KRaft mode, so I also removed related tests. ping @hachikuji to have a look, thank you 🤝

@dengziming

Copy link
Copy Markdown
Member Author

Hello @mumrah , are you interested in reviewing this, currently, APiVersionRequest is not properly handled in KRaft server and will have a bad effect on KIP-778.

@dengziming

Copy link
Copy Markdown
Member Author

Hello @mumrah , are you interested in reviewing this? currently, APiVersionRequest is not properly handled in KRaft server and will have a bad effect on KIP-778.

@dengziming dengziming changed the title KAFKA-13228: ApiVersionRequest is not properly handled in KRaft KAFKA-13228: ApiVersionRequest is not properly handled in KRaft co-resident mode Jan 7, 2022
@dengziming dengziming closed this Feb 17, 2022
@ijuma

ijuma commented Jun 12, 2022

Copy link
Copy Markdown
Member

Was this fixed some other way?

@dengziming

Copy link
Copy Markdown
Member Author

Was this fixed some other way?

No, there are too many conflicts, I split this PR into 2 PRs, #12157 and #11784.

@ijuma

ijuma commented Jun 13, 2022

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants