[WFCORE-6739] Allow ordering of ServerActivity execution by registerin… #5884

bstansberry · 2024-02-26T21:48:02Z

…g them in distinct, ordered, 'execution groups'

https://issues.redhat.com/browse/WFCORE-6739

This is a draft pending further discussion, particularly of how suitable this is for the transaction subsystem's implementation of https://issues.redhat.com/browse/WFLY-17742.

@jmfinelli FYI.

wildfly-ci · 2024-02-27T02:49:08Z

Core -> Full Integration Build 13523 outcome was FAILURE using a merge of 804b199
Summary: Tests failed: 1 (1 new), passed: 7712, ignored: 119 Build time: 04:54:01

Failed tests

org.jboss.as.test.clustering.cluster.ejb.timer.DistributedTimerServiceTestCase.test: java.lang.AssertionError: node-2: org.jboss.as.test.clustering.cluster.ejb.timer.beans.SingleActionPersistentTimerBean expected:<1> but was:<0>
	at org.jboss.as.test.clustering.cluster.ejb.timer.AbstractTimerServiceTestCase.test(AbstractTimerServiceTestCase.java:95)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor35.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor34.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor33.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor5.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor3.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor15.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor14.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor5.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at jdk.internal.reflect.GeneratedMethodAccessor3.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
------- Stdout: -------
node-2  [0m23:28:59,482 INFO  [org.jboss.as.repository] (management-handler-thread - 1) WFLYDR0001: Content added at location /opt/buildAgent/work/e8e0dd9c7c4ba60/full/testsuite/integration/clustering/target/wildfly-2/standalone/data/content/cd/3b78b7aaa08820f35d18457b34945d6804dbcc/content
 [0mnode-2  [0m23:28:59,487 INFO  [org.jboss.as.server.deployment] (MSC service thread 1-5) WFLYSRV0027: Starting deployment of "DistributedTimerServiceTestCase.war" (runtime-name: "DistributedTimerServiceTestCase.war")
 [0mnode-2  [0m23:28:59,903 INFO  [org.jboss.weld.deployer] (MSC service thread 1-7) WFLYWELD0003: Processing weld deployment DistributedTimerServiceTestCase.war
 [0mnode-2  [0m23:28:59,919 INFO  [org.jboss.as.ejb3.deployment] (MSC service thread 1-7) WFLYEJB0473: JNDI bindings for session bean named 'AutoTransientTimerBean' in deployment unit 'deployment "DistributedTimerServiceTestCase.war"' are as follows:

	java:global/DistributedTimerServiceTestCase/AutoTransientTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.TimerBean
	java:app/DistributedTimerServiceTestCase/AutoTransientTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.TimerBean
	java:module/AutoTransientTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.TimerBean
	java:global/DistributedTimerServiceTestCase/AutoTransientTimerBean
	java:app/DistributedTimerServiceTestCase/AutoTransientTimerBean
	java:module/AutoTransientTimerBean

 [0mnode-2  [0m23:28:59,919 INFO  [org.jboss.as.ejb3.deployment] (MSC service thread 1-7) WFLYEJB0473: JNDI bindings for session bean named 'SingleActionPersistentTimerBean' in deployment unit 'deployment "DistributedTimerServiceTestCase.war"' are as follows:

	java:global/DistributedTimerServiceTestCase/SingleActionPersistentTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.ManualTimerBean
	java:app/DistributedTimerServiceTestCase/SingleActionPersistentTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.ManualTimerBean
	java:module/SingleActionPersistentTimerBean!org.jboss.as.test.clustering.cluster.ejb.timer.beans.ManualTimerBean
	java:global/DistributedTimerServiceTestCase/SingleActionPersistentTimerBean
	java:app/DistributedTimerServiceTestCase/SingleActionPersistentTimerBean
	java:module/SingleActionPersistentTimerBean

 [0mnode-2  [0m23:28:59,919 INFO  [org.jboss.as.ejb3.deployment] (MSC service thread 1-7) WFLYEJB0473: JNDI bindings for session bean named 'AutoPersistentTimerBean' in deployment unit 'deployment "DistributedTimerServiceTestCase.war"' are as follows:

jmfinelli

I'm not sure why this is a DRAFT PR :-) It looks super good to me.
I just left a comment in the code.

Thank you very much @bstansberry for taking care of this

jmfinelli · 2024-03-13T17:28:51Z

server/src/main/java/org/jboss/as/server/suspend/SuspendController.java

@@ -75,20 +81,32 @@ public synchronized void suspend(long timeoutMillis) {
        for(OperationListener listener: new ArrayList<>(operationListeners)) {
            listener.suspendStarted();
        }
-        outstandingCount = activities.size();
-        if (outstandingCount == 0) {
+        groupsCount = activities.size();


Shouldn't this be

Suggested change

groupsCount = activities.size();

// Work out the number of ServerActivity implementations

activities.forEach((ranking,executionGroup) -> groupsCount =+ executionGroup.size());

Or, we could also add a groupsCount++ in registerActivity(ServerActivity)

I don't think so. This var is meant to track the number of groups, not the number of activities. When each group completes preSuspend work it signals preSuspendGroupCallBack. When preSuspendGroupCallBack get groupsCount signals, it proceeds with the 'suspend' phase.

I should probably rename the 'activities' field to 'activitiesByGroup'.

Yep, I see it now. Sorry for the noise.

bstansberry · 2024-03-13T20:33:34Z

Thanks for the review, @jmfinelli.

It's draft for a couple reasons. :-)

The biggest is that we're doing this to meet your use case, so before having other folks waste time looking at it, I wanted to make sure you thought it would. When we talked I thought we understood each other and something like this should solve your problem, but maybe I misunderstood or you thought more and don't think it will help etc. Sounds like it's ok though, so that's good.

The other is that I should write a unit test. This works fine it that it doesn't break existing tests but that doesn't prove it actually works. :)

Looking at SuspendController it seems very unit-testable, and the basic behavior that needs testing is straightforward (activities are invoked in the expected order regardless of the order of calls to registerActivity).

jmfinelli · 2024-03-14T10:12:12Z

Yep, it look super good to me and it reflects exactly what we talked about during the meeting :-) I think this is the last piece of the puzzle 🥳 I also declared a dependency in the proposal.

…g them in distinct, ordered, 'execution groups'

bstansberry · 2024-03-15T01:01:51Z

@jmfinelli I added the test and moved this out of draft state.

bstansberry · 2024-03-18T19:34:16Z

Thanks @jmfinelli

github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Feb 26, 2024

bstansberry force-pushed the WFLY-19067 branch from 6e0b22a to 804b199 Compare February 26, 2024 21:57

jmfinelli reviewed Mar 13, 2024

View reviewed changes

bstansberry changed the title ~~[WFLY-19067] Allow ordering of ServerActivity execution by registerin…~~ [WFCORE-6739] Allow ordering of ServerActivity execution by registerin… Mar 13, 2024

bstansberry force-pushed the WFLY-19067 branch from 804b199 to fa8048a Compare March 15, 2024 00:53

[WFLY-19067] Allow ordering of ServerActivity execution by registerin…

7b9ab0d

…g them in distinct, ordered, 'execution groups'

bstansberry force-pushed the WFLY-19067 branch from fa8048a to 7b9ab0d Compare March 15, 2024 01:01

bstansberry marked this pull request as ready for review March 15, 2024 01:01

jmfinelli approved these changes Mar 15, 2024

View reviewed changes

bstansberry merged commit 45e4a25 into wildfly:main Mar 18, 2024
12 checks passed

bstansberry deleted the WFLY-19067 branch March 18, 2024 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFCORE-6739] Allow ordering of ServerActivity execution by registerin… #5884

[WFCORE-6739] Allow ordering of ServerActivity execution by registerin… #5884

bstansberry commented Feb 26, 2024 •

edited

Loading

wildfly-ci commented Feb 27, 2024

jmfinelli left a comment

jmfinelli Mar 13, 2024

bstansberry Mar 13, 2024

jmfinelli Mar 14, 2024

bstansberry commented Mar 13, 2024

jmfinelli commented Mar 14, 2024

bstansberry commented Mar 15, 2024

bstansberry commented Mar 18, 2024

	groupsCount = activities.size();
	// Work out the number of ServerActivity implementations
	activities.forEach((ranking,executionGroup) -> groupsCount =+ executionGroup.size());

[WFCORE-6739] Allow ordering of ServerActivity execution by registerin… #5884

[WFCORE-6739] Allow ordering of ServerActivity execution by registerin… #5884

Conversation

bstansberry commented Feb 26, 2024 • edited Loading

wildfly-ci commented Feb 27, 2024

Failed tests

jmfinelli left a comment

Choose a reason for hiding this comment

jmfinelli Mar 13, 2024

Choose a reason for hiding this comment

bstansberry Mar 13, 2024

Choose a reason for hiding this comment

jmfinelli Mar 14, 2024

Choose a reason for hiding this comment

bstansberry commented Mar 13, 2024

jmfinelli commented Mar 14, 2024

bstansberry commented Mar 15, 2024

bstansberry commented Mar 18, 2024

bstansberry commented Feb 26, 2024 •

edited

Loading