ISPN-3197 Message ordering of Get and Invalidation can cause L1 to be in... #1922
Conversation
| @@ -62,44 +69,219 @@ | |||
| private CommandsFactory cf; | |||
| private LockManager lockManager; | |||
| private EntryFactory entryFactory; | |||
| private DataContainer dataContainer; | |||
|
|
|||
| private final ConcurrentMap<Object, L1ReadSynchronizer> concurrentReads = new ConcurrentHashMap<Object, L1ReadSynchronizer>(); | |||
There was a problem hiding this comment.
Please use CollectionFactory.makeConcurrentMap instead. It allows us to swap between the JDK's CHM and the new CHMv8 which we've backported.
There was a problem hiding this comment.
Ah thanks for pointing that out.
|
@danberindei @mmarkus This pull req requires careful review. |
|
Yes, as many eyes as can be spared would be best for this 👍 Unfortunately this was the simplest way I could find out of all the things I tried, that covered various inconsistency edge cases. One thing to note is that this fix will need to be ported over to tx L1 as well at some point as it has the same issues, so if there are any pointers there you can think of, let me know! |
|
I did a quick look at the discussion and I have some comments:
wdyt? |
|
@pruivo Except the remote get may be actually over, and that thread is now updating the entry in the data container. I think that's the meaning of the RUNNING state... For write commands we now decide whether to write a regular entry or an L1 entry in EntryWrappingInterceptor... doing another write in another interceptor would require us to acquire the shared state transfer lock again. |
|
@danberindei I see... However, I think that all the events should synchronize at some point (update by remote get, update by local put and invalidation). |
|
I am attempting to write up a dev list to nicely summarize and will include the latest comments to start it off. |
|
@danberindei Updated it with most recent changes. |
| Object returnValue; | ||
| if (ctx.isOriginLocal()) { | ||
| Object key = command.getKey(); | ||
| // If the value isn't possible to return a remote value - don't single request it |
There was a problem hiding this comment.
I didn't really understand the comment
There was a problem hiding this comment.
When I mention single request, I am basically referring to the "corralling" part. Essentially if we know the value isn't going to look remotely or is already in the data container it just lets the invocation go down the interceptor chain.
There was a problem hiding this comment.
Do you think something like is better?
// If the command isn't going to return a remote value - just pass it down the interceptor chain
|
I fixed up rework items. Some comments still persist. Also this discussion is not confirmed good, but I removed the catch block #1922 (comment) |
… inconsistent. * Implemented single entry point for concurrent gets. * Also support concurrent invalidation for reads with limited blocking * Added tests for various concurrency/edge issues
|
Changed up comments so it says whether the get worked or not. Also added in static variable to hold whether trace is enabled. |
|
Looks good, pulling. |
|
Integrated, thanks @wburns! |
...consistent.