sunchao commented on a change in pull request #2110:
URL: https://github.com/apache/hadoop/pull/2110#discussion_r448757877
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -726,4 +741,86 @@ public TokenIdent decodeTokenIdentifier(Token<TokenIdent>
token) throws IOExcept
return token.decodeIdentifier();
}
+ /**
+ * Return top token real owners list as well as the tokens count.
+ *
+ * @param n top number of users
+ * @return map of owners to counts
+ */
+ public List<NameValuePair> getTopTokenRealOwners(int n) {
Review comment:
It's slightly unfortunate that we need to expose `NameValuePair` in a
public method here as it is scope = private, but I don't know an easy way to
avoid this.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -726,4 +741,86 @@ public TokenIdent decodeTokenIdentifier(Token<TokenIdent>
token) throws IOExcept
return token.decodeIdentifier();
}
+ /**
+ * Return top token real owners list as well as the tokens count.
+ *
+ * @param n top number of users
+ * @return map of owners to counts
+ */
+ public List<NameValuePair> getTopTokenRealOwners(int n) {
+ n = Math.min(n, tokenOwnerStats.size());
+ if (n == 0) {
+ return new ArrayList<>();
+ }
+
+ TopN topN = new TopN(n);
+ for (Map.Entry<String, Long> entry : tokenOwnerStats.entrySet()) {
+ topN.offer(new NameValuePair(
+ entry.getKey(), entry.getValue()));
+ }
+
+ List<NameValuePair> list = new ArrayList<>();
+ while (!topN.isEmpty()) {
+ list.add(topN.poll());
+ }
+ Collections.reverse(list);
+ return list;
+ }
+
+ /**
+ * Return the real owner for a token. If this is a token from a proxy user,
+ * the real/effective user will be returned.
+ *
+ * @param id
+ * @return real owner
+ */
+ public String getTokenRealOwner(TokenIdent id) {
+ String realUser;
+ if (id.getRealUser() != null && !id.getRealUser().toString().isEmpty()) {
+ realUser = id.getRealUser().toString();
+ } else {
+ // if there is no real user -> this is a non proxy user
+ // the user itself is the real owner
+ realUser = id.getUser().getUserName();
+ }
+ return realUser;
+ }
+
+ /**
+ * Add token stats to the owner to token count mapping.
+ *
+ * @param id
+ */
+ public void addTokenForOwnerStats(TokenIdent id) {
+ String realOwner = getTokenRealOwner(id);
+ tokenOwnerStats.put(realOwner,
+ tokenOwnerStats.getOrDefault(realOwner, 0l)+1);
Review comment:
nit: use capital `0L` to make IDE happy
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -578,6 +591,7 @@ public synchronized TokenIdent
cancelToken(Token<TokenIdent> token,
if (info == null) {
throw new InvalidToken("Token not found " + formatTokenId(id));
}
+ removeTokenForOwnerStats(id);
Review comment:
same here - swap order and update stats as the last step?
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -64,7 +69,13 @@ private String formatTokenId(TokenIdent id) {
*/
protected final Map<TokenIdent, DelegationTokenInformation> currentTokens
= new ConcurrentHashMap<>();
-
+
+ /**
+ * Map of token real owners to its token count. This is used to generate
+ * top users by owned tokens.
+ */
+ protected final Map<String, Long> tokenOwnerStats = new
ConcurrentHashMap<>();
Review comment:
Seems this won't be updated if a standby NN updates its own token info
by pulling from edit log?
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/resources/hdfs-rbf-default.xml
##########
@@ -657,4 +657,12 @@
</description>
</property>
+ <property>
+ <name>dfs.federation.router.top.num.token.realowners</name>
+ <value>10</value>
+ <description>
+ The number of top real owners by tokens count to report in the JMX
metrics.
Review comment:
nit: might explain a little bit what "real owners" mean here
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -292,6 +303,7 @@ protected DelegationTokenInformation
getTokenInfo(TokenIdent ident) {
protected void storeToken(TokenIdent ident,
DelegationTokenInformation tokenInfo) throws IOException {
currentTokens.put(ident, tokenInfo);
+ addTokenForOwnerStats(ident);
Review comment:
can we swap the order and update the stats as last step?
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -64,7 +69,13 @@ private String formatTokenId(TokenIdent id) {
*/
protected final Map<TokenIdent, DelegationTokenInformation> currentTokens
= new ConcurrentHashMap<>();
-
+
+ /**
+ * Map of token real owners to its token count. This is used to generate
+ * top users by owned tokens.
Review comment:
`top users` -> `top user metrics`?
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -726,4 +741,86 @@ public TokenIdent decodeTokenIdentifier(Token<TokenIdent>
token) throws IOExcept
return token.decodeIdentifier();
}
+ /**
+ * Return top token real owners list as well as the tokens count.
+ *
+ * @param n top number of users
+ * @return map of owners to counts
+ */
+ public List<NameValuePair> getTopTokenRealOwners(int n) {
+ n = Math.min(n, tokenOwnerStats.size());
+ if (n == 0) {
+ return new ArrayList<>();
+ }
+
+ TopN topN = new TopN(n);
+ for (Map.Entry<String, Long> entry : tokenOwnerStats.entrySet()) {
+ topN.offer(new NameValuePair(
+ entry.getKey(), entry.getValue()));
+ }
+
+ List<NameValuePair> list = new ArrayList<>();
+ while (!topN.isEmpty()) {
+ list.add(topN.poll());
+ }
+ Collections.reverse(list);
+ return list;
+ }
+
+ /**
+ * Return the real owner for a token. If this is a token from a proxy user,
+ * the real/effective user will be returned.
+ *
+ * @param id
+ * @return real owner
+ */
+ public String getTokenRealOwner(TokenIdent id) {
+ String realUser;
+ if (id.getRealUser() != null && !id.getRealUser().toString().isEmpty()) {
+ realUser = id.getRealUser().toString();
+ } else {
+ // if there is no real user -> this is a non proxy user
+ // the user itself is the real owner
+ realUser = id.getUser().getUserName();
+ }
+ return realUser;
+ }
+
+ /**
+ * Add token stats to the owner to token count mapping.
+ *
+ * @param id
+ */
+ public void addTokenForOwnerStats(TokenIdent id) {
+ String realOwner = getTokenRealOwner(id);
+ tokenOwnerStats.put(realOwner,
+ tokenOwnerStats.getOrDefault(realOwner, 0l)+1);
+ }
+
+ /**
+ * Remove token stats to the owner to token count mapping.
+ *
+ * @param id
+ */
+ public void removeTokenForOwnerStats(TokenIdent id) {
+ String realOwner = getTokenRealOwner(id);
+ if (tokenOwnerStats.containsKey(realOwner)) {
+ // unlikely to be less than 1 but in case
+ if (tokenOwnerStats.get(realOwner) <= 1) {
Review comment:
Even though `tokenOwnerStats` is a concurrent map, you may run into race
conditions if multiple threads operate on this method at the same time. We can
potentially make the method synchronized to avoid that. Not sure we should care
much though since this is only for metrics.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
##########
@@ -726,4 +741,86 @@ public TokenIdent decodeTokenIdentifier(Token<TokenIdent>
token) throws IOExcept
return token.decodeIdentifier();
}
+ /**
+ * Return top token real owners list as well as the tokens count.
+ *
+ * @param n top number of users
+ * @return map of owners to counts
+ */
+ public List<NameValuePair> getTopTokenRealOwners(int n) {
+ n = Math.min(n, tokenOwnerStats.size());
+ if (n == 0) {
+ return new ArrayList<>();
+ }
+
+ TopN topN = new TopN(n);
+ for (Map.Entry<String, Long> entry : tokenOwnerStats.entrySet()) {
+ topN.offer(new NameValuePair(
+ entry.getKey(), entry.getValue()));
+ }
+
+ List<NameValuePair> list = new ArrayList<>();
+ while (!topN.isEmpty()) {
+ list.add(topN.poll());
+ }
+ Collections.reverse(list);
+ return list;
+ }
+
+ /**
+ * Return the real owner for a token. If this is a token from a proxy user,
+ * the real/effective user will be returned.
+ *
+ * @param id
+ * @return real owner
+ */
+ public String getTokenRealOwner(TokenIdent id) {
+ String realUser;
+ if (id.getRealUser() != null && !id.getRealUser().toString().isEmpty()) {
+ realUser = id.getRealUser().toString();
+ } else {
+ // if there is no real user -> this is a non proxy user
+ // the user itself is the real owner
+ realUser = id.getUser().getUserName();
+ }
+ return realUser;
+ }
+
+ /**
+ * Add token stats to the owner to token count mapping.
+ *
+ * @param id
+ */
+ public void addTokenForOwnerStats(TokenIdent id) {
+ String realOwner = getTokenRealOwner(id);
+ tokenOwnerStats.put(realOwner,
+ tokenOwnerStats.getOrDefault(realOwner, 0l)+1);
+ }
+
+ /**
+ * Remove token stats to the owner to token count mapping.
+ *
+ * @param id
+ */
+ public void removeTokenForOwnerStats(TokenIdent id) {
Review comment:
This and a few others do not need to be public?
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
