-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
KAFKA-18399: Remove ZooKeeper from KafkaApis (3/N): USER_SCRAM_CREDENTIALS #18456
Conversation
@@ -3370,10 +3370,7 @@ class KafkaApis(val requestChannel: RequestChannel, | |||
} else { | |||
metadataSupport match { | |||
case ZkSupport(adminManager, controller, zkClient, forwardingManager, metadataCache, _) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we will eventually remove zk related code, probably we can use case default
here and avoid to use ZkSupport
.
183998e
to
aed36ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
aed36ec
to
6c48c3d
Compare
requestHelper.sendResponseMaybeThrottle(request, requestThrottleMs => | ||
alterUserScramCredentialsRequest.getErrorResponse(requestThrottleMs, Errors.CLUSTER_AUTHORIZATION_FAILED.exception)) | ||
} | ||
throw KafkaApis.shouldAlwaysForward(request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing I said here: https://github.com/apache/kafka/pull/18465/files#r1910472130
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frankvicky Could you please refactor maybeForwardToController
to forwardToController
? We will remove all zk-releated handlers, so it is fine to do refactor in this PR. With that change, the handle handleAlterUserScramCredentialsRequest
can be removed directly
6c48c3d
to
feaafc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for this fix.
dda5314
to
99fe806
Compare
99fe806
to
3c9d632
Compare
@@ -3173,37 +3161,16 @@ class KafkaApis(val requestChannel: RequestChannel, | |||
describeUserScramCredentialsRequest.getErrorResponse(requestThrottleMs, Errors.CLUSTER_AUTHORIZATION_FAILED.exception)) | |||
} else { | |||
metadataSupport match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please change the type of metadataSupport
to RaftSupport
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, that will be handled by #18483. please ignore above comment
3c9d632
to
c203485
Compare
…IALS (#18456) Reviewers: Chia-Ping Tsai <[email protected]>
…IALS (apache#18456) Reviewers: Chia-Ping Tsai <[email protected]>
…IALS (apache#18456) Reviewers: Chia-Ping Tsai <[email protected]>
…IALS (apache#18456) Reviewers: Chia-Ping Tsai <[email protected]>
JIRA: KAFKA-18399
This is a part of KAFKA-18399 and it's focus on clean up
ALTER_USER_SCRAM_CREDENTIALS
andDESCRIBE_USER_SCRAM_CREDENTIALS
Committer Checklist (excluded from commit message)