[GitHub] storm issue #2622: STORM-3020: fix possible race condition in AsyncLocalizer
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2622 @HeartSaVioR thanks for the review. I fixed your concerns by making them all ConcurrentHashMaps and adding a note about why they need to be that. I could not find a good way to remove the sideeffects. ---
[GitHub] storm issue #2622: STORM-3020: fix possible race condition in AsyncLocalizer
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2622 @HeartSaVioR Great catch I forgot to update the normal has Map/HashMap to a ConcurrentHashMap. Yes the guarantees of ConcurrentMap allow for retry and we do have side effects in some of the computes. I will update the comments as well to say what requirements we have for the type. The computeIfAbsent methods have no side effects, so we don't need to worry about them as much. I'll see if I can come up with a way to make it so we don't need as strong of a guarantee for `compute` so perhaps we could use other implementations. ---
[GitHub] storm issue #2622: STORM-3020: fix possible race condition in AsyncLocalizer
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2622 @revans2 Could you elaborate how protection works? At the first time I thought it leverages atomicity of compute*, but reading description of compute* in Map, looks like guaranteeing atomicity is not forced to implementations. https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#compute-K-java.util.function.BiFunction- and HashMap doesn't mention about atomicity (`topologyBlobs` is a HashMap): https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#compute-K-java.util.function.BiFunction- whereas ConcurrentHashMap mentions about atomicity so at least this guarantees atomicity: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#compute-K-java.util.function.BiFunction- It may worth noting that implementation requirement in ConcurrentMap looks like a bit different from what ConcurrentHashMap provides. `The default implementation may retry these steps when multiple threads attempt updates including potentially calling the remapping function multiple times.` It doesn't mention locking but retrying. https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentMap.html#compute-K-java.util.function.BiFunction- ---
[GitHub] storm issue #2622: STORM-3020: fix possible race condition in AsyncLocalizer
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2622 Thanks for the review @danny0405 This was not trying to fix STORM-2905. This was a separate race condition I found when reviewing your pull request for STORM-2905. ---
[GitHub] storm issue #2622: STORM-3020: fix possible race condition in AsyncLocalizer
Github user danny0405 commented on the issue: https://github.com/apache/storm/pull/2622 I test it, and it worked ok as before, but a KeyNotFoundException still got of master when finally cleaned the base blobs. ---
