[GitHub] [druid] imply-cheddar commented on pull request #13049: Synchronize query context getMergedParams()

[GitBox <gi...@apache.org>]

imply-cheddar commented on PR #13049:
URL: https://github.com/apache/druid/pull/13049#issuecomment-1240038290

   What was the stack trace that this is now fixing?  I'm curious as to the source of the NPE...
   
   I say that because races inside of that `if` should still set the value to be non-null, so a race there shouldn't generate an NPE.  I just noticed that `mergedParams` is not marked as `volatile`, so it could actually be an issue with memory cache lines across cores as the non-volatile update wouldn't force a cache-line update.  Just changing the `if` clause to return `merged` (something guaranteed to be non-null) instead of rolling out to the final `return mergedParams` would probably actually achieve the same result.  That said, the more better fix for thise would be to make `mergedParams` volatile given that it is changing (or, even better, make it an `AtomicReference<Map<>>` to make it super explicit that we expect multiple threads).
   
   A perhaps even nicer fix would be a `DelegatingMap` class or something like that which just wraps the chain of Maps and does the resolution at read time instead.  In that world we could create it as a final at construction time and just reuse that.  But, if such a class doesn't already exist, creating it and proving that it is correct is a lot more difficult than just switching to an `AtomicReference`. 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org