pprovenzano commented on code in PR #14083:
URL: https://github.com/apache/kafka/pull/14083#discussion_r1282518474
##########
core/src/main/scala/kafka/server/DelegationTokenManager.scala:
##########
@@ -165,10 +112,11 @@ object DelegationTokenManager {
class DelegationTokenManager(val config: KafkaConfig,
Review Comment:
> I think the functionality of this class has changed drastically as in the
new implementation it seems like to be a cache only. Having
DelegationTokenManagerZk extending this is a little bit confusing as you need
to throw exceptions in methods that this class really would never use. What do
you think about refactoring this a bit and split it into 2 parts:
>
> * a cache that has a zk and a kraft impl
> * a forwarder that also has a zk and a kraft impl
>
> You can then initialize it accordingly in `BrokerServer` and `KafkaServer`
accordingly.
See the answer in the next comment.
One additional reason this doesn't work is that KafkaApi needs to access the
TokenCache via a DelegationTokenManager for both Zk and Kraft so having a Zk
and Kraft implementation would need a base class anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
