pzampino commented on a change in pull request #339:
URL: https://github.com/apache/knox/pull/339#discussion_r436275056
##########
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -100,7 +100,10 @@ protected void persistTokenState() {
for (TokenState state : processing) {
tokenIds.add(state.getTokenId());
aliases.put(state.getAlias(), state.getAliasValue());
- log.creatingTokenStateAliases(state.getTokenId());
Review comment:
> Not part of this code change but i do not follow the previous
`synchronized() `block, we have processing array in the synchronized block that
is also outside the block.
> Wouldn't it be easier and safe to make `processing` and `unpersistedState`
arrays (and other if any) threadsafe? e.g.
> `Collections.synchronizedList(unpersistedState)`
> `Collections.synchronizedList(unpersistedState)`
The _processing_ List is populated from the shared resource
(_unpersistedState_) in a synchronized block, such that it is populated in a
thread-safe way. Subsequently, some work is performed based on the contents of
_processing_, but being a local variable, it need not be synchronized. The
_unpersistedState_ can be augmented by other threads without any impact to this
work of creating the aliases. If the alias creation fails, _unpersistedState_
is again locked to re-add the state for which the aliases could not be added,
so it will be attempted on the next iteration. I don't see any reason to lock
_unpersistedState_ while the aliases are being created, and it actually would
have a detrimental effect on the rate at which tokens are issued because
keypair generation is so costly.
----------------------------------------------------------------
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]
