ISPN-9511 Expired event is not fired when modifying an expired entry #6260

wburns · 2018-09-18T17:26:15Z

https://issues.jboss.org/browse/ISPN-9511

wburns · 2018-09-19T01:52:41Z

Upon thinking about this further, the fix for ISPN-9530 is not correct.

The following line https://github.com/infinispan/infinispan/blob/master/server/hotrod/src/main/java/org/infinispan/server/hotrod/ClientListenerRegistry.java#L206 should really use regular registerFilterListener. I need to look into fixing it when a converter is passed, why it didn't work before. My guess is that it applied the encoding is done twice. I will have to look further.

wburns · 2018-09-19T21:02:16Z

I will need to rebase this after #6263 is integrated. However it should hopefully pass CI now.

wburns · 2018-10-03T16:27:41Z

Tests passing, rebased to latest master and added some more docs

danberindei

Will, do we have a description somewhere of how clustered expiration is supposed to work? I barely remember anything I've learned about it earlier, and it keeps changing :)

danberindei · 2018-10-03T19:08:30Z

core/src/main/java/org/infinispan/commands/write/RemoveExpiredCommand.java

-      // Override the flags
-      return FlagBitSets.SKIP_CACHE_LOAD;
+      // Override the flags to always include skip cache load
+      return flags | FlagBitSets.SKIP_CACHE_LOAD;


Maybe it would be nicer to add the SKIP_CACHE_LOAD flag in CacheImpl

Hrmm, okay. Let me try to reorg it.

danberindei · 2018-10-03T19:17:41Z

core/src/main/java/org/infinispan/container/impl/EntryFactoryImpl.java

         if (trace)
            log.tracef("Retrieved from container %s", ice);
         if (ice == null) {
            return NullCacheEntry.getInstance();
         }
         return ice;
      } else if (isL1Enabled) {
-         final InternalCacheEntry ice = innerGetFromContainer(key, segment, writeOperation, false);
+         final InternalCacheEntry ice = innerGetFromContainer(key, segment, false, isPrimaryOwner(isOwner, segment));


Could it really be a primary owner if it wasn't an owner?

No, but it seemed a little cleaner than adding an && here and hard coding a value in isPrimaryOwner. But either way.

I meant we're already on the else branch of if (isOwner), so isPrimaryOwner(isOwner, segment) == false.

I was going to change it yeah :)

Gotcha, I wasn't look at which branch it was on 👍

danberindei · 2018-10-03T19:21:39Z

core/src/main/java/org/infinispan/container/impl/EntryFactoryImpl.java

+            // actually expire the entry from memory
+            if (isPrimaryOwner) {
+               // This method is always called from a write operation
+               expirationManager.entryExpiredInMemory(ice, currentTime, true).join();


I'm not sure about this, it feels like we're spreading the expiration logic in too many places. And wrapping entries shouldn't block...

Where would you recommend the logic should go? I don't have a good spot since we don't want this in the DataContainer. Unless you think we should move the other logic out of the DataContainer into here?

Wrapping entries already blocks for max idle :)

Why not in the DataContainer? It already deals with expiration...

Although moving the expiration logic somewhere where it could be made non-blocking does sound better.

Sorry I missed the blocking in the max idle PR review :P

I responded quickly to run out, but the more I am thinking I would rather probably have this in EntryFactoryImpl and make both methods non blocking with interceptor stack. Although I haven't looked at details if this is possible.

Now that I got a chance to look at this, I am not sure how to make this non blocking without changing the EntryFactory interface. What do you think @danberindei ? You have had a lot more experience changing these things.

Yeah, making it non-blocking is a lot more involved. It's in an impl package so changing the interface is not a big deal, but I wouldn't push for it in 9.4.

Not sure if moving everything expiration-related to EntryFactoryImpl right now is feasible either, that's why my first thought was to keep everything in the data container.

Yeah to add it to the DataContainer would require changing that interface, which is a lot more difficult to do. So I would rather move it into EntryFactoryImpl at some point and eventually try to push that to the EntryWrappingInterceptor, but I don't see a nice easy way to do that. I would say what is here is probably what will have to be for 9.4.0.

Ok for now :)

danberindei · 2018-10-03T19:43:48Z

core/src/main/java/org/infinispan/expiration/impl/ClusterExpirationManager.java

@@ -126,25 +127,28 @@ private void addAndWaitIfFull(CompletableFuture future, List<CompletableFuture>
      }
   }

-   CompletableFuture<Void> handleLifespanExpireEntry(K key, V value, long lifespan) {
+   // Skip locking is available in case if the entry was found expired due to a write, which would already
+   // have the lock and remove lifespan expired is fired async


I'm a bit confused... If the RemoveExpiredCommand is fired async, why would it need to skip locking? Also, does available mean true here?

It needs to skip locking, because it has to remove the expired entry and generate the event before the current write can occur. The current write has the lock, so the async remove expired can't acquire locks. It waits for the remove to complete causing them to be serialized and guarantees event ordering so the expiration event comes before the write event.

Ok, but if we block the current write until the RemoveExpiredCommand finishes running, I wouldn't really call it async, even if it does run on another thread.

technically it is async from this method :)

Sure, but that doesn't explain why it needs to skip locking :)

Sure, it just was supposed to say required instead of available. The problem is that the write already has the lock as it mentioned. I can add another sentence stating that it could cause a deadlock.

I think the confusion is because the blocking is done by the caller. I will also add a note mentioning that when skipLocking is true it is up to the caller to properly ensure that the lock is held for the entire duration.

I reworded and changed variable names so it should be more clear now.

Thanks, it's a little more detail than I wanted now, but still much clearer :)

danberindei · 2018-10-03T19:48:51Z

core/src/main/java/org/infinispan/expiration/impl/ClusterExpirationManager.java

-         handleLifespanExpireEntry(entry.getKey(), value, lifespan);
+         // If the entry as found expired in memory during a write, that means we already have the lock for the
+         // given entry. Thus we must skip locking and wait for the remove expired to complete before continuing
+         // forward with the operation to let the event be raised properly.


Do we guarantee that expiration listeners are notified synchronously with the read/write that triggered the expiration, or does "the event be raised properly" mean something else?

yes they are done in a serial manner, expiration event has to come before the write event.

danberindei · 2018-10-03T19:49:48Z

core/src/main/java/org/infinispan/interceptors/locking/NonTransactionalLockingInterceptor.java

@@ -40,6 +41,11 @@ protected Object visitDataWriteCommand(InvocationContext ctx, DataWriteCommand c
      return visitNonTxDataWriteCommand(ctx, command);
   }

+   @Override
+   public Object visitRemoveExpiredCommand(InvocationContext ctx, RemoveExpiredCommand command) throws Throwable {
+      return super.visitRemoveExpiredCommand(ctx, command);


Is this really needed?

No :) Was just there to add a debug break point and forgot to remove.

danberindei · 2018-10-03T21:25:48Z

core/src/main/java/org/infinispan/commands/read/AbstractDataCommand.java

@@ -16,7 +16,7 @@
 */
 public abstract class AbstractDataCommand implements DataCommand, SegmentSpecificCommand {
   protected Object key;
-   private long flags;
+   protected long flags;


No longer needed

danberindei · 2018-10-03T21:28:36Z

core/src/main/java/org/infinispan/container/impl/EntryFactoryImpl.java

+      if (distributionManager != null) {
+         return distributionManager.getCacheTopology().getSegmentDistribution(segment).isPrimary();
+      } else {
+         return isOwner;


Actually distributionManager == null only in LOCAL caches, so you can return true here and remove the parameter.

danberindei · 2018-10-05T10:35:03Z

Integrated, thanks Will!
The build failure was caused by ISPN-9588.

wburns requested review from gustavocoding and danberindei September 18, 2018 17:26

wburns added Changes Required Preview labels Sep 18, 2018

wburns force-pushed the ISPN-9511_expired_event_on_write branch 2 times, most recently from 38c953a to aecbb29 Compare September 19, 2018 21:01

wburns added Needs Rebase and removed Changes Required Preview labels Sep 19, 2018

wburns force-pushed the ISPN-9511_expired_event_on_write branch from aecbb29 to aceaecd Compare September 20, 2018 14:23

wburns removed the Needs Rebase label Sep 20, 2018

wburns added this to the 9.4.0.Final milestone Sep 20, 2018

tristantarrant changed the title ~~ISPN-9511 Expired event is not raise when modifying an expired entry~~ ISPN-9511 Expired event is not fired when modifying an expired entry Sep 28, 2018

wburns force-pushed the ISPN-9511_expired_event_on_write branch 2 times, most recently from 562ea5e to b438ea7 Compare September 28, 2018 19:49

wburns requested review from tristantarrant and removed request for gustavocoding October 3, 2018 16:23

wburns force-pushed the ISPN-9511_expired_event_on_write branch from b438ea7 to 51e4078 Compare October 3, 2018 16:27

danberindei reviewed Oct 3, 2018

View reviewed changes

wburns force-pushed the ISPN-9511_expired_event_on_write branch from 51e4078 to 7627763 Compare October 3, 2018 21:11

danberindei reviewed Oct 3, 2018

View reviewed changes

wburns force-pushed the ISPN-9511_expired_event_on_write branch 2 times, most recently from efd0dae to f30f028 Compare October 4, 2018 12:51

ISPN-9511 Expired event is not fired when modifying an expired entry

4ad07b4

wburns force-pushed the ISPN-9511_expired_event_on_write branch from f30f028 to 4ad07b4 Compare October 4, 2018 14:48

danberindei merged commit 518ca2a into infinispan:master Oct 5, 2018

ISPN-9511 Expired event is not fired when modifying an expired entry #6260

ISPN-9511 Expired event is not fired when modifying an expired entry #6260

Conversation

wburns commented Sep 18, 2018 • edited

wburns commented Sep 19, 2018

wburns commented Sep 19, 2018

wburns commented Oct 3, 2018

danberindei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danberindei Oct 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danberindei commented Oct 5, 2018

wburns commented Sep 18, 2018 •

edited

danberindei Oct 3, 2018 •

edited