lirui-apache commented on a change in pull request #15810:
URL: https://github.com/apache/flink/pull/15810#discussion_r630117107
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/security/modules/HadoopModule.java
##########
@@ -97,8 +97,7 @@ public void install() throws SecurityInstallException {
// If UGI use keytab for login, do not load HDFS
delegation token.
for (Token<? extends TokenIdentifier> token : usrTok) {
if (!token.getKind().equals(hdfsDelegationTokenKind)) {
- final Text id = new Text(token.getIdentifier());
- credentialsToBeAdded.addToken(id, token);
+ credentialsToBeAdded.addToken(token.getService(),
token);
Review comment:
Hey @ChengbingLiu , thanks for the explanations. I'm not very familiar
with this topic. So I have some follow-up questions.
1. Per HDFS-9276, the deep copy is added to avoid scenarios like `"Whenever
the content of token.service is changed, publicService will reflect the change.
I don't think this is what we want."` So I wonder whether we need to guard
against similar situations. E.g. when `token.service` is changed, do we want it
to be reflected in the `Credentials::tokenMap`?
2. The `usrTok` is indeed local in `HadoopModule`. However, a similar change
is made in `Utils::setTokensFor` where `usrTok` is retrieved from current UGI
and therefore is not local to the method. Will that be an issue for us?
3. It seems both `Text(byte[])` and `Text(Text)` copy the byte array. So
previously to this change, we do make a deep copy, no?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
