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]
