WFLY-12321 #12449

fl4via · 2019-07-24T22:49:32Z

Do not merge yet

Jira: https://issues.jboss.org/browse/WFLY-12321

This PR has to incorporate a few more feedbacks and will most likely be broken into more PRs and Jiras.
EAP Jira (temporary): https://issues.jboss.org/browse/JBEAP-17049

...jb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/InfinispanBeanManager.java

clustering/api/src/main/java/org/wildfly/clustering/dispatcher/CommandDispatcher.java

.../infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/BeanExpirationScheduler.java

pferraro · 2019-07-25T12:32:06Z

.../infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/BeanExpirationScheduler.java

+                return;
+            }
+            task = expireTask;
+            expireTask = null;


Should we be concerned that the expireTask can be reset after this synchronized block completes? Whose cancels the Future in that case?

I agree, I fixed that by checking if expireTask is different from null at the end of the file, before trying to reschedule it and reset the field.

...jb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/InfinispanBeanManager.java

ejb3/src/main/java/org/jboss/as/ejb3/cache/simple/SimpleCache.java

fl4via · 2019-07-29T14:11:50Z

@pferraro I've updated the PR with all your feedback. Also, I removed the commit for "At LocalCommandDispatcher for clustering, executeOnMember should check if this node is equal to member, and not if it is equal to itself". I was thinking it shoudl have its own Jira, dont you agree? Or do you prefer to have the commit at this PR as it was before?

pferraro · 2019-07-29T14:24:02Z

@fl4via

I've updated the PR with all your feedback. Also, I removed the commit for "At LocalCommandDispatcher for clustering, executeOnMember should check if this node is equal to member, and not if it is equal to itself". I was thinking it shoudl have its own Jira, dont you agree? Or do you prefer to have the commit at this PR as it was before?

Agreed. I merged it last week.
#12445

...rc/test/java/org/jboss/as/test/clustering/cluster/dispatcher/bean/CommandDispatcherBean.java

clustering/ejb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/Scheduler.java

pferraro · 2019-07-31T14:31:42Z

...rc/main/java/org/wildfly/clustering/ejb/infinispan/PrepareReeschedulingSchedulerCommand.java

+ * the near future.
+ * @author Flavia Rainone
+ */
+public class PrepareReeschedulingSchedulerCommand<I> implements Command<Void, Scheduler<I>> {


Spelling fix: PrepareReschedulingSchedulerCommand :)

Ooops, thanks :-)

rhusar · 2019-07-31T20:27:20Z

With the latest changes org.jboss.as.test.clustering.cluster.ejb.stateful.StatefulTimeoutTestCase#timeout fails consistently.

fl4via · 2019-07-31T21:31:45Z

Retest this please

pferraro · 2019-07-31T21:08:49Z

...jb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/InfinispanBeanManager.java

    private void cancel(Bean<I, T> bean) {
        if (this.dispatcher != null) {
+            final Node node = locatePrimaryOwner(bean.getId());


Remove this line.

pferraro · 2019-07-31T21:08:57Z

...jb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/InfinispanBeanManager.java

@@ -219,8 +218,28 @@ public Affinity getWeakAffinity(I id) {
        return Affinity.NONE;
    }

+    private void prepareRescheduling(Bean<I, T> bean) {
+        if (this.dispatcher != null) {
+            final Node node = locatePrimaryOwner(bean.getId());


Remove this line.

pferraro · 2019-07-31T23:22:46Z

.../infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/BeanExpirationScheduler.java

    }

    @Override
    public void cancel(Locality locality) {
-        for (I id: this.expirationFutures.keySet()) {
+        for (I id: expirationTracker.getTrackedIds()) {


Iterating over this set will be prone to ConcurrentModificationExceptions.

pferraro · 2019-07-31T23:23:23Z

...b/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/EagerEvictionScheduler.java

    }

    @Override
    public void cancel(Locality locality) {
-        for (I id: this.evictionFutures.keySet()) {
+        for (I id: this.expirationTracker.getTrackedIds()) {


Iterating over this set will be prone to ConcurrentModificationExceptions.

stuartwdouglas · 2019-08-01T07:58:30Z

...ng/ejb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/ExpirationTracker.java

+                        firstExpiration != lastExpiration) {
+                    firstExpiration = firstExpiration.nextExpiration;
+                }
+                expiration.previousExpiration = null;


the nextExpriation must also me cleared, or an endless loop can result

pferraro · 2019-08-01T17:50:35Z

...ng/ejb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/ExpirationTracker.java

+                firstExpiration = lastExpiration = expiration;
+            } else if (lastExpiration != expiration) {
+                // if the last expiration to occur is not current node, add it to the
+                // last of the expiration list


This logic makes the assumption that the newly tracked bean will be the last to expire. While this is true when scheduling a bean for expiration at the end of an invocation, this is generally not the case when a member takes on the responsibility for expiring a given bean, i.e. when triggered from InfinispanBeanManager.schedule(Locality oldLocality, Locality newLocality).

Oh, I see. I'll try to think on a solution and I'll assert the perf gains with the benchmark to make sure this PR is really needed.

One solution is to replace the expiration tracker with a collection, e.g. io.undertow.util.ConcurrentDirectDeque. We can then construct the scheduler with a queue implementation appropriate for the use case. In the clustered case, where the assumption above is violated, we could use a priority queue, where elements are inserted according to their next expiration, rather then just appending to the end. Insertions/removals would be slower, i.e. log(N) instead of constant time, but that way we can still optimize for the local case.

fl4via · 2019-08-23T18:34:41Z

@pferraro this commit is ready for review

fl4via · 2019-08-23T18:35:33Z

Retest this please

fl4via · 2019-08-23T20:53:20Z

Retest this please

fl4via · 2019-08-23T20:56:39Z

Retest this please

fl4via · 2019-08-29T19:14:24Z

...jb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/InfinispanBeanManager.java

+            try {
+                this.executeOnPrimaryOwner(id, new PrepareReschedulingSchedulerCommand<>(id));
+            } catch (Exception e) {
+                InfinispanEjbLogger.ROOT_LOGGER.failedToCancelBean(e, id);


@pferraro I didn t think it was worthy to add a new logging message for something that will be later removed. If you prefer to have a logging message created for this, let me know.

bstansberry · 2019-09-15T03:07:53Z

@fl4via @pferraro Is this meant for WF 18?

pferraro · 2019-09-16T12:19:47Z

.../src/main/java/org/wildfly/clustering/ejb/infinispan/NonClusteredEagerEvictionScheduler.java

+    public void evict(I id) {
+        try {
+            // Cache eviction is a local operation, so we need to broadcast this to the cluster
+            this.dispatcher.executeOnGroup(new EvictCommand<>(id));


Given that this is now a local-only scheduler, there is no need to dispatch an eviction command.

pferraro · 2019-09-16T12:24:46Z

@bstansberry This is a performance optimization for local SFSBs. Not essential for WF18, but still desirable.

@param

…r type @param comment

pferraro · 2019-09-16T21:31:29Z

.../src/main/java/org/wildfly/clustering/ejb/infinispan/NonClusteredEagerEvictionScheduler.java

+        this.executor = executor;
+        this.idleTimeout = idleTimeout;
+        this.expirationTracker = new ExpirationTracker<>(idleTimeout);
+        this.dispatcher = dispatcherFactory.createCommandDispatcher(dispatcherName + "/eviction", evictor);


This scheduler no longer has any need to create a CommandDispatcher, no?

pferraro · 2019-09-16T21:32:34Z

.../src/main/java/org/wildfly/clustering/ejb/infinispan/NonClusteredEagerEvictionScheduler.java

+        // do nothing, this is a local-only scheduler
+    }
+
+    static class EvictCommand<I> implements Command<Void, BeanGroupEvictor<I>> {


I don't think this class is referenced any longer.

pferraro · 2019-09-16T21:34:10Z

.../src/main/java/org/wildfly/clustering/ejb/infinispan/NonClusteredEagerEvictionScheduler.java

+
+    @Override
+    public void evict(I id) {
+        // do nothing, this is a local-only scheduler


This method needs to evict the bean, i.e. this.evictor.evict(id);

…a single task for eviction and another for bean expiration and never cancel that task; instead, update the expiration of the bean so it can be checked by the task if the bean has actually expired

fl4via · 2019-09-17T06:37:10Z

@pferraro all requested changes are done

pferraro

Thanks Flavia. I think this is ready now.

pferraro requested changes Jul 25, 2019

View reviewed changes

fl4via force-pushed the WFLY-12321 branch from d7f38d7 to f597bc8 Compare July 29, 2019 14:08

wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 29, 2019

fl4via force-pushed the WFLY-12321 branch 2 times, most recently from 4e2b959 to 605d0b3 Compare July 30, 2019 20:22

fl4via marked this pull request as ready for review July 30, 2019 20:41

pferraro requested changes Jul 31, 2019

View reviewed changes

...rc/test/java/org/jboss/as/test/clustering/cluster/dispatcher/bean/CommandDispatcherBean.java Outdated Show resolved Hide resolved

clustering/ejb/infinispan/src/main/java/org/wildfly/clustering/ejb/infinispan/Scheduler.java Show resolved Hide resolved

fl4via force-pushed the WFLY-12321 branch from 605d0b3 to 3a91277 Compare July 31, 2019 13:48

pferraro reviewed Jul 31, 2019

View reviewed changes

pferraro requested changes Jul 31, 2019

View reviewed changes

stuartwdouglas reviewed Aug 1, 2019

View reviewed changes

pferraro requested changes Aug 1, 2019

View reviewed changes

fl4via force-pushed the WFLY-12321 branch from 8f1715b to e109481 Compare August 23, 2019 18:33

fl4via force-pushed the WFLY-12321 branch from e109481 to cbe29c8 Compare August 23, 2019 20:53

fl4via force-pushed the WFLY-12321 branch 2 times, most recently from 0f4a9a8 to 1178575 Compare August 23, 2019 20:56

fl4via force-pushed the WFLY-12321 branch from 1178575 to bcd3c0b Compare August 29, 2019 19:12

fl4via commented Aug 29, 2019

View reviewed changes

fl4via force-pushed the WFLY-12321 branch from bcd3c0b to 39b4dc6 Compare August 30, 2019 06:45

pferraro requested changes Sep 16, 2019

View reviewed changes

pferraro self-requested a review September 16, 2019 12:22

At several clustering infinispan classes, remove the G group identfie…

3498467

…r type @param comment

fl4via force-pushed the WFLY-12321 branch from 39b4dc6 to 66ef462 Compare September 16, 2019 21:21

pferraro requested changes Sep 16, 2019

View reviewed changes

[WFLY-12321] At InfinispanBeanManager, if node is singleton schedule …

3ca3384

…a single task for eviction and another for bean expiration and never cancel that task; instead, update the expiration of the bean so it can be checked by the task if the bean has actually expired

fl4via force-pushed the WFLY-12321 branch from 66ef462 to 3ca3384 Compare September 17, 2019 06:35

pferraro approved these changes Sep 17, 2019

View reviewed changes

bstansberry added the ready-for-merge Only for use by those with merge permissions! label Sep 17, 2019

bstansberry mentioned this pull request Sep 17, 2019

Pulls #12658

Closed

bstansberry merged commit c2d61d9 into wildfly:master Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WFLY-12321 #12449

WFLY-12321 #12449

fl4via commented Jul 24, 2019

pferraro Jul 25, 2019

fl4via Jul 31, 2019

fl4via commented Jul 29, 2019

pferraro commented Jul 29, 2019

pferraro Jul 31, 2019

fl4via Jul 31, 2019

rhusar commented Jul 31, 2019

fl4via commented Jul 31, 2019

pferraro Jul 31, 2019

pferraro Jul 31, 2019

pferraro Jul 31, 2019

pferraro Jul 31, 2019

stuartwdouglas Aug 1, 2019

pferraro Aug 1, 2019

fl4via Aug 13, 2019

pferraro Aug 13, 2019 •

edited

Loading

fl4via commented Aug 23, 2019

fl4via commented Aug 23, 2019

fl4via commented Aug 23, 2019

fl4via commented Aug 23, 2019

fl4via Aug 29, 2019

bstansberry commented Sep 15, 2019

pferraro Sep 16, 2019

pferraro commented Sep 16, 2019

pferraro Sep 16, 2019

fl4via Sep 17, 2019

pferraro Sep 16, 2019

fl4via Sep 17, 2019

pferraro Sep 16, 2019

fl4via Sep 17, 2019

fl4via commented Sep 17, 2019

pferraro left a comment

WFLY-12321 #12449

WFLY-12321 #12449

Conversation

fl4via commented Jul 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fl4via commented Jul 29, 2019

pferraro commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhusar commented Jul 31, 2019

fl4via commented Jul 31, 2019

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

pferraro Aug 13, 2019 • edited Loading

Choose a reason for hiding this comment

fl4via commented Aug 23, 2019

fl4via commented Aug 23, 2019

fl4via commented Aug 23, 2019

fl4via commented Aug 23, 2019

Choose a reason for hiding this comment

bstansberry commented Sep 15, 2019

Choose a reason for hiding this comment

pferraro commented Sep 16, 2019

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

fl4via commented Sep 17, 2019

pferraro left a comment

Choose a reason for hiding this comment

pferraro Aug 13, 2019 •

edited

Loading