KAFKA-6447: Add Delegation Token Operations to KafkaAdminClient (KIP-249)#4427
KAFKA-6447: Add Delegation Token Operations to KafkaAdminClient (KIP-249)#4427junrao merged 4 commits intoapache:trunkfrom
Conversation
omkreddy
changed the title
|
retest this please |
|
@hachikuji @ijuma @rajinisivaram sorry for bothering you. Pl take a look at |
omkreddy
force-pushed
the
KAFKA-6447
branch
3 times, most recently
from
e6eb95d to
09ffd41
Compare
|
@junrao @rajinisivaram please take a look when you get a chance. Thanks. |
There was a problem hiding this comment.
We probably want to document that if owners is null, all delegation tokens will be returned?
There was a problem hiding this comment.
Since DelegationToken is part of the public api, we need to add org.apache.kafka.common.security.token.delegation to the following section in build.gradle.
javadoc {
include "**/org/apache/kafka/clients/admin/*"
include "**/org/apache/kafka/clients/consumer/*"
include "**/org/apache/kafka/clients/producer/*"
include "**/org/apache/kafka/common/*"
include "**/org/apache/kafka/common/acl/*"
include "**/org/apache/kafka/common/annotation/*"
include "**/org/apache/kafka/common/errors/*"
include "**/org/apache/kafka/common/header/*"
include "**/org/apache/kafka/common/resource/*"
include "**/org/apache/kafka/common/serialization/*"
include "**/org/apache/kafka/common/config/*"
include "**/org/apache/kafka/common/security/auth/*"
include "**/org/apache/kafka/server/policy/*"
}
There was a problem hiding this comment.
done. moved couple of internal classes to internal package.
There was a problem hiding this comment.
Should we just remove all delegation token related methods from this scala class?
There was a problem hiding this comment.
yes, I think we can delete those methods. since scala client is deprecated and this feature is released recently, hope it wont create any compatibility issues.
There was a problem hiding this comment.
Do we need to convert tokenInfo.issueTimestamp to Date before calling format()?
There was a problem hiding this comment.
we can pass epoch time milliseconds to dateFormat.format(Object o) method.
https://docs.oracle.com/javase/7/docs/api/java/text/DateFormat.html#format(java.lang.Object,%20java.lang.StringBuffer,%20java.text.FieldPosition)
There was a problem hiding this comment.
Do we allow the tool to list all delegation tokens? It would be useful to document this from the command line option and also add a test for that.
There was a problem hiding this comment.
updated the tool to return all user tokens, if the ownerPrincipals are not supplied.
Added a test class for DelegationTokenCommand
There was a problem hiding this comment.
Should we also assert that the return list has size 1?
There was a problem hiding this comment.
With the default option, the owner list will be empty. In this case, it seems the broker will return an empty list instead of the tokens for the owner?
There was a problem hiding this comment.
With the default option, the owner list will be null. broker will return user owned tokens and tokens where user have Describe permission. This added to the Javadoc.
omkreddy
force-pushed
the
KAFKA-6447
branch
2 times, most recently
from
81e7f4b to
7718247
Compare
There was a problem hiding this comment.
It seems that we can just reference tokens and get rid of describeResult.
There was a problem hiding this comment.
@junrao Thanks for the latest review. Addressed the review comments.
…249) (apache#4427) Reviewers: Jun Rao <junrao@gmail.com>
…249) (apache#4427) Reviewers: Jun Rao <junrao@gmail.com>
…249) (apache#4427) Reviewers: Jun Rao <junrao@gmail.com>
…249) (apache#4427) Reviewers: Jun Rao <junrao@gmail.com>
2 participants
Committer Checklist (excluded from commit message)