[
https://issues.apache.org/jira/browse/KNOX-2375?focusedWorklogId=439956&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-439956
]
ASF GitHub Bot logged work on KNOX-2375:
----------------------------------------
Author: ASF GitHub Bot
Created on: 02/Jun/20 07:32
Start Date: 02/Jun/20 07:32
Worklog Time Spent: 10m
Work Description: smolnar82 commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433675431
##########
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -319,16 +366,17 @@ protected void evictExpiredTokens() {
*/
protected boolean needsEviction(final String tokenId) throws
UnknownTokenException {
// If the expiration time(+ grace period) has already passed, it should be
considered expired
- long expirationWithGrace = getTokenExpiration(tokenId) +
TimeUnit.SECONDS.toMillis(tokenEvictionGracePeriod);
+ long expirationWithGrace = getTokenExpiration(tokenId, false) +
TimeUnit.SECONDS.toMillis(tokenEvictionGracePeriod);
return (expirationWithGrace <= System.currentTimeMillis());
}
/**
* Get a list of tokens
*
- * @return List of tokens
+ * @return
*/
protected List<String> getTokens() {
- return new ArrayList<>(tokenExpirations.keySet());
+ return tokenExpirations.keySet().stream().collect(Collectors.toList());
Review comment:
Should not we synchronize here too?
##########
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -237,12 +249,37 @@ protected boolean isUnknown(final String token) {
protected void updateExpiration(final String tokenId, long expiration) {
synchronized (tokenExpirations) {
- tokenExpirations.replace(tokenId, expiration);
+ if (tokenExpirations.containsKey(tokenId)) {
+ tokenExpirations.replace(tokenId, expiration);
+ } else {
+ tokenExpirations.put(tokenId, expiration);
+ }
Review comment:
`Map.replace` also runs a `containsKey`:
```
The default implementation is equivalent to, for this map:
if (map.containsKey(key)) {
return map.put(key, value);
} else
return null;
```
I think a simple `put` is enough: you want to make sure you have the given
expiration in `tokenExpirations`
##########
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -109,30 +133,52 @@ public long getTokenExpiration(final String tokenId)
throws UnknownTokenExceptio
@Override
protected boolean isUnknown(final String tokenId) {
- boolean isUnknown = false;
- try {
- isUnknown =
(aliasService.getPasswordFromAliasForCluster(AliasService.NO_CLUSTER_NAME,
tokenId) == null);
- } catch (AliasServiceException e) {
- log.errorAccessingTokenState(tokenId, e);
+ boolean isUnknown = super.isUnknown(tokenId);
+
+ // If it's not in the cache, then check the underlying alias
+ if (isUnknown) {
+ try {
+ isUnknown =
(aliasService.getPasswordFromAliasForCluster(AliasService.NO_CLUSTER_NAME,
tokenId) == null);
+ } catch (AliasServiceException e) {
+ log.errorAccessingTokenState(tokenId, e);
+ }
}
return isUnknown;
}
@Override
protected void removeToken(final String tokenId) throws
UnknownTokenException {
- validateToken(tokenId);
-
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,
tokenId);
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId
+ TOKEN_MAX_LIFETIME_POSTFIX);
log.removedTokenState(tokenId);
} catch (AliasServiceException e) {
log.failedToRemoveTokenState(tokenId, e);
}
+ super.removeToken(tokenId);
+ }
Review comment:
Why not invoke the new method as
`removeTokens(Collections.singleton(tokenId))`?
##########
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -254,7 +291,7 @@ protected void removeToken(final String tokenId) throws
UnknownTokenException {
protected boolean hasRemainingRenewals(final String tokenId, long
renewInterval) {
// Is the current time + 30-second buffer + the renewal interval is less
than the max lifetime for the token?
- return ((System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(30) +
renewInterval) < getMaxLifetime(tokenId));
+ return ((System.currentTimeMillis() + 30000 + renewInterval) <
getMaxLifetime(tokenId));
Review comment:
Why is this 30 seconds buffer added?
##########
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -237,12 +249,37 @@ protected boolean isUnknown(final String token) {
protected void updateExpiration(final String tokenId, long expiration) {
synchronized (tokenExpirations) {
- tokenExpirations.replace(tokenId, expiration);
+ if (tokenExpirations.containsKey(tokenId)) {
+ tokenExpirations.replace(tokenId, expiration);
+ } else {
+ tokenExpirations.put(tokenId, expiration);
+ }
}
}
protected void removeToken(final String tokenId) throws
UnknownTokenException {
validateToken(tokenId);
+ removeTokenState(tokenId);
+ }
+
+ /**
+ * Bulk removal of the specified tokens.
+ *
+ * @param tokenIds The unique identifiers of the tokens whose state should
be removed.
+ *
+ * @throws UnknownTokenException
+ */
+ protected void removeTokens(final Set<String> tokenIds) throws
UnknownTokenException {
+ // Duplicating the logic from removeToken(String) here because this method
is supposed to be an optimization for
+ // sub-classes that access an external store, for which bulk token removal
performs better than individual removal.
+ // Sub-classes will have implemented removeToken(String), and calling that
here will undo any optimizations provided
+ // by the sub-class's implementation of this method.
+ for (String tokenId : tokenIds) {
+ removeTokenState(tokenId);
+ }
+ }
+
+ private void removeTokenState(final String tokenId) {
Review comment:
I'm not sure if I agree that any non-optimized code should be left if we
already know that code piece will result in causing performance issues.
My two cents here:
1. in `removeToken` I'd call `removeTokens(Collections.singleton(tokenId))`
2. in `removeTokens` I'd **not** call a synchronized block `N` times.
Instead, you may synchronize once, fetch the keyset and call the `removeAll` on
that key set -> that collection is
> backed by the map, so changes to the map are reflected in the set, and
vice-versa.
Something like this:
```
synchronized (tokenExpirations) {
tokenExpirations.keySet().removeAll(tokenIds);
}
```
----------------------------------------------------------------
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]
Issue Time Tracking
-------------------
Worklog Id: (was: 439956)
Time Spent: 20m (was: 10m)
> Token state eviction should access the keystore file less frequently
> --------------------------------------------------------------------
>
> Key: KNOX-2375
> URL: https://issues.apache.org/jira/browse/KNOX-2375
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
> Affects Versions: 1.4.0
> Reporter: Philip Zampino
> Assignee: Philip Zampino
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> When the AliasBasedTokenStateService is employed, the TokenStateService
> reaper loads the keystore file (via the AliasService and KeyStoreService)
> very frequently.
> # It queries all the token-state-related aliases
> # For every token ID
> ## Looks up the token again (validateToken())
> ## Looks up the the token expiration
> ## Removes the token expiration alias
> ## Removes the token max lifetime alias
> This means the KeyStoreService loads the keystore file (1 + 2-to-4-per-token)
> times every eviction interval (default 5 minutes). That means, if there are
> 100 expired tokens and 100 unexpired tokens, the reaper will load the
> keystore file 601 times in one iteration.
> As the keystore file size increases, the already poor performance of loading
> this file degrades even more to the point that the token state reaper can
> consume 100% of the CPU.
> The reaper should operate on the in-memory token state as much as possible,
> and even remove expired token state in bulk (loading / writing the keystore
> file once for all).
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
