Skip to content

KAFKA-12365; Disable APIs not supported by KIP-500 broker/controller#10194

Merged
hachikuji merged 5 commits intoapache:trunkfrom
hachikuji:KAFKA-12365
Feb 26, 2021
Merged

KAFKA-12365; Disable APIs not supported by KIP-500 broker/controller#10194
hachikuji merged 5 commits intoapache:trunkfrom
hachikuji:KAFKA-12365

Conversation

@hachikuji
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji commented Feb 23, 2021

This patch updates request listeners tags to be in line with what the KIP-500 broker/controller support today. We will re-enable these APIs as needed once we have added the support.

I have also updated ControllerApis to use ApiVersionManager and simplified the envelope handling logic.

Committer Checklist (excluded from commit message)

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

@hachikuji
Copy link
Copy Markdown
Contributor Author

Note I'm doing a pass over the other APIs that should be disabled for now. For example, the reassignment APIs are not yet supported. I will probably just generalize this patch to cover all of them.

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Feb 25, 2021

The approach LGTM to me. Can you also disable the relevant system test(s) in this PR?

Agree that we need to disable a few more things -- reassignment is one

@hachikuji hachikuji changed the title KAFKA-12365; Disable transactional APIs on KIP-500 broker KAFKA-12365; Disable APIs not supported by KIP-500 broker/controller Feb 25, 2021
"apiKey": 48,
"type": "request",
"listeners": ["zkBroker", "broker"],
"listeners": ["zkBroker"],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

describeClientQuotas is implemented for kip-500 already (on the broker)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm... looking at trunk, it looks like there are some changes from the KIP-500 branch that didn't make it over. handleDescribeClientQuotasRequest did work in the kip-500 branch. Anyway, at this point, I think we should just disable it and put client quotas support on the TODO list for the next release (sigh)

Comment thread clients/src/main/resources/common/message/DeleteTopicsRequest.json Outdated
}
new ApiVersionsResponse(data)
} else {
apiVersionManager.apiVersionResponse(requestThrottleMs)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we do need to fill in the feature fields eventually. But for 2.8, this is OK.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right. I think the change makes sense in any case since the feature logic will end up getting handled by ApiVersionManager (as we do with the broker).

Copy link
Copy Markdown
Contributor

@cmccabe cmccabe Feb 26, 2021

Choose a reason for hiding this comment

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

agreed. we will have to have some way for the controller to update those fields in ApiVersionsManager (possibly an atomic / volatile field or something).

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

@hachikuji hachikuji merged commit 74dfe80 into apache:trunk Feb 26, 2021
hachikuji added a commit that referenced this pull request Feb 26, 2021
…10194)

This patch updates request `listeners` tags to be in line with what the KIP-500 broker/controller support today. We will re-enable these APIs as needed once we have added the support.

We have also updated `ControllerApis` to use `ApiVersionManager` and simplified the envelope handling logic.

Reviewers: Ron Dagostino <rdagostino@confluent.io>, Colin P. McCabe <cmccabe@apache.org>
@ijuma ijuma added the kraft label Aug 11, 2021
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