ISPN-7911 Functional commands do not invalidate L1 cache #5197
Conversation
| Object KEY = getKeyForCache(primary, backup); | ||
| primary.put(KEY, "value"); | ||
|
|
||
| assertEquals(null, reader.getAdvancedCache().getDataContainer().get(KEY)); |
There was a problem hiding this comment.
I would, but regrettably AssertJUnit then says only 'value was expected to be null' and does not specify that; It's more useful for debugging to use assertEquals(null, ...)
| assertEquals(null, reader.getAdvancedCache().getDataContainer().get(KEY)); | ||
| assertEquals("value", reader.get(KEY)); | ||
|
|
||
| assertEntry(primary.getAdvancedCache().getDataContainer().get(KEY), "value", false); |
There was a problem hiding this comment.
can this getAdvancedCache().getDataContainer().get(KEY) be done inside assertEntry method ?
| assertEntry(primary.getAdvancedCache().getDataContainer().get(KEY), "value", false); | ||
| assertEntry(backup.getAdvancedCache().getDataContainer().get(KEY), "value", false); | ||
| assertEntry(reader.getAdvancedCache().getDataContainer().get(KEY), "value", true); | ||
| assertEquals(null, nonOwner.getAdvancedCache().getDataContainer().get(KEY)); |
|
|
||
| assertEntry(primary.getAdvancedCache().getDataContainer().get(KEY), "value2", false); | ||
| assertEntry(backup.getAdvancedCache().getDataContainer().get(KEY), "value2", false); | ||
| assertEquals(null, reader.getAdvancedCache().getDataContainer().get(KEY)); |
|
Cleaned up. |
|
Looks good. CI didn't complete last time for other reasons, let's wait for it. |
|
@rvansa I took the commit to continue my tests on the multimap and I realized the FunctionalL1Test does not compile due to the PR where you moved the API from commons to core |
|
@karesti Thanks, updated. |
| return asyncValue(f.handle((nil, throwable) -> { | ||
| // Ignore SuspectExceptions - if the node has gone away then there is nothing to invalidate anyway. | ||
| if (throwable != null && !(throwable.getCause() instanceof SuspectException)) { | ||
| getLog().failedInvalidatingRemoteCache(throwable); |
There was a problem hiding this comment.
flushCache always uses SYNC_IGNORE_LEAVERS, so this isn't necessary. SYNC would return too early anyway, if we wanted to log invalidations that failed because nodes left we'd have to use a ResponseFilter.
There was a problem hiding this comment.
Ok, I was merely refactoring the code. I'll remove this.
|
The interceptors are not really async; |
|
Rebased, removed the two checks for |
|
@rvansa I see, |
| return rv; | ||
| } | ||
| return asyncValue(f.handle((nil, throwable) -> { | ||
| // Ignore SuspectExceptions - if the node has gone away then there is nothing to invalidate anyway. |
There was a problem hiding this comment.
The comment still mentions SuspectException`.
TBH I didn't read the code close enough, I just assumed exceptions other than SuspectException fail the write because one node now sees a stale value, and that you'd be able to remove asyncReturnValue altogether. WDYT?
There was a problem hiding this comment.
The original code just logged the exception and continued. I can propagate the failure, but the method can't be ridden of because we need to swap the return value to rv.
There was a problem hiding this comment.
I was hoping for an opinion on whether to continue or to propagate the failure :)
However, in the meantime I found that L1NonTxInterceptor already propagates the exception, so I propose doing the same in L1TxInterceptor and in L1LastChanceInterceptor.
|
OK, rethrowing exceptions. |
|
@rvansa the FunctionalL1Test does not compile anymore 😢 |
* includes minor refactoring of L1-related interceptors to remove synchronous waiting
|
😡 fixed |
|
Integrated, thanks Radim! |
https://issues.jboss.org/browse/ISPN-7911