You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2020/09/08 17:52:00 UTC
[jira] [Work logged] (KNOX-2408) Improve AliasBasedTokenState
service performance
[ https://issues.apache.org/jira/browse/KNOX-2408?focusedWorklogId=480324&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-480324 ]
ASF GitHub Bot logged work on KNOX-2408:
----------------------------------------
Author: ASF GitHub Bot
Created on: 08/Sep/20 17:51
Start Date: 08/Sep/20 17:51
Worklog Time Spent: 10m
Work Description: pzampino commented on a change in pull request #371:
URL: https://github.com/apache/knox/pull/371#discussion_r480391677
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -344,36 +320,44 @@ protected void validateToken(final String tokenId) throws IllegalArgumentExcepti
}
}
- protected String getTimestampDisplay(long timestamp) {
+ private String getTimestampDisplay(long timestamp) {
return Instant.ofEpochMilli(timestamp).toString();
}
/**
* Method that deletes expired tokens based on the token timestamp.
*/
protected void evictExpiredTokens() {
- Set<String> tokensToEvict = new HashSet<>();
-
- for (final String tokenId : getTokens()) {
- try {
- if (needsEviction(tokenId)) {
- log.evictToken(tokenId);
- tokensToEvict.add(tokenId); // Add the token to the set of tokens to evict
+ if (readyForEviction()) {
+ final Set<String> tokensToEvict = new HashSet<>();
+
+ for (final String tokenId : getTokenIds()) {
+ try {
+ if (needsEviction(tokenId)) {
+ log.evictToken(tokenId);
+ tokensToEvict.add(tokenId); // Add the token to the set of tokens to evict
+ }
+ } catch (final Exception e) {
+ log.failedExpiredTokenEviction(tokenId, e);
}
- } catch (final Exception e) {
- log.failedExpiredTokenEviction(tokenId, e);
}
- }
- if (!tokensToEvict.isEmpty()) {
- try {
- removeTokens(tokensToEvict);
- } catch (UnknownTokenException e) {
- log.failedExpiredTokenEviction(e);
+ if (!tokensToEvict.isEmpty()) {
+ try {
+ removeTokens(tokensToEvict);
+ } catch (UnknownTokenException e) {
+ log.failedExpiredTokenEviction(e);
+ }
}
+ } else {
+ log.skipEviction();
}
}
+ protected boolean readyForEviction() {
+ return true;
Review comment:
Why is this method (and associated invocation) required if the answer is always 'true'?
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/services/security/AliasService.java
##########
@@ -54,6 +54,8 @@ void generateAliasForCluster(String clusterName, String alias)
char[] getPasswordFromAliasForGateway(String alias)
throws AliasServiceException;
+ Map<String, char[]> getPasswordAliasMapForGateway() throws AliasServiceException;
Review comment:
Why not simply getGatewayAliases(), more similar to other signatures in this interface? Why the need to cite the return type in the method signature?
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -122,6 +129,44 @@ public void start() throws ServiceLifecycleException {
statePersistenceInterval,
TimeUnit.SECONDS);
}
+
+ // Loading ALL entries from __gateway-credentials.jceks could be VERY time-consuming (it took a bit more than 19 minutes to load 12k aliases
+ // during my tests).
+ // Therefore, it's safer to do it in a background thread than just make the service start hang until it's finished
+ final ExecutorService gatewayCredentialsLoader = Executors.newSingleThreadExecutor(new BasicThreadFactory.Builder().namingPattern("GatewayCredentialsLoader").build());
+ gatewayCredentialsLoader.execute(this::loadGatewayCredentialsOnStartup);
+ }
+
+ private void loadGatewayCredentialsOnStartup() {
+ try {
+ log.loadingGatewayCredentialsOnStartup();
+ final long start = System.currentTimeMillis();
+ final Map<String, char[]> passwordAliasMap = aliasService.getPasswordAliasMapForGateway();
+ String alias, tokenId;
+ long expiration, maxLifeTime;
+ int count = 0;
+ for (Map.Entry<String, char[]> passwordAliasMapEntry : passwordAliasMap.entrySet()) {
+ alias = passwordAliasMapEntry.getKey();
+ if (alias.endsWith(TOKEN_MAX_LIFETIME_POSTFIX)) {
Review comment:
Some comments here might be helpful for future viewers. I understand what is being done here, but others may not.
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultAliasService.java
##########
@@ -194,6 +195,24 @@ public void removeAliasesForCluster(String clusterName, Set<String> aliases) thr
return getPasswordFromAliasForCluster(NO_CLUSTER_NAME, alias);
}
+ //Overriding the default behavior as we want to avoid loading the keystore N-times from the file system
+ @Override
Review comment:
Does something similar need to be done for cluster-specific aliases?
----------------------------------------------------------------
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:
users@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 480324)
Time Spent: 0.5h (was: 20m)
> Improve AliasBasedTokenState service performance
> ------------------------------------------------
>
> Key: KNOX-2408
> URL: https://issues.apache.org/jira/browse/KNOX-2408
> Project: Apache Knox
> Issue Type: Task
> Components: Server
> Affects Versions: 1.4.0
> Reporter: Sandor Molnar
> Assignee: Sandor Molnar
> Priority: Major
> Fix For: 1.5.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> While working on KNOX-2402 there were some performance issues related to {{AliasBasedTokenStateService}} which the new tool revealed during the test phase:
> {{AliasBasedTokenStateService}}:
> - {{updateExpiration()}} goes directly to alias service rather than updating it locally and let the background thread does its job
> - {{getTokens}} goes directly to alias service
> {{DefaultTokenStateService}}:
> - we should start using {{ConcurrentHashMap}} instead of {{HashMap}} for {{tokenExpirations}} and {{maxTokenLifetimes}} -> many of the synchronization blocks could be get rid of (CHM gives better performance anyway)
> - review {{getTokens()}} usage
> - eviction: needs to consider the case when Gateway was restarted -> nothing found in memory -> eviction is extremely slow
--
This message was sent by Atlassian Jira
(v8.3.4#803005)