INT-2595: Add time-based `ReleaseStrategy` option #1100

artembilan · 2014-04-02T11:03:47Z

JIRA: https://jira.spring.io/browse/INT-2595

Add group-timeout and group-timeout-expression to the Correlation Endpoint
Add logic to the AbstractCorrelatingMessageHandler to schedule group for forceComplete,
when the target ReleaseStrategy returns false

artembilan · 2014-04-02T11:07:02Z

Let me know, if it's OK and I'll add some docs.

artembilan · 2014-04-02T11:21:56Z

Travis failed with OutOfMemory and looks like it really needs your commit with Gradle options.

garyrussell · 2014-04-02T13:18:29Z

OK; when we merge my PR, rebase and push -f this branch and travis will re-build.

garyrussell · 2014-04-02T14:33:49Z

...ion-core/src/main/java/org/springframework/integration/aggregator/ReaperReleaseStrategy.java

+
+	public ReaperReleaseStrategy(ReleaseStrategy delegate, TaskScheduler taskScheduler, MessageGroupStore store,
+								 long interval, long timeout) {
+		this.delegate = delegate;


We should probably assert that the delegate is not an RRR.

RRR 😄 . Right, I was thinking to make it as package protected, but then I won't be able to configure it from DSL and I've protected us from this in JavaDocs.

garyrussell · 2014-04-02T14:53:25Z

This is really just a convenience for configuring the reaper - avoids a separate bean and scheduled task.

If we stick with this technique, we should rename the attributes reaper-interval, reaper-group-timeout.

However, the alternative I was thinking of was to put logic in the ACMH.

Have a ScheduledFuture<?> for each group in a Map. When a new message arrives, cancel the schedule for this group (if it exists), add the message to the group, if the ReleaseStrategy doesn't fire, schedule a new timeout task for this group.

That way, we would only need group-timeout (or maybe even also group-timeout-expression). We wouldn't need an interval. This would have the added benefit in that group timeouts would be more accurate - currently group timeouts are only approximate - depending on the reaper's schedule.

garyrussell · 2014-04-02T14:55:56Z

...ore/src/main/resources/org/springframework/integration/config/xml/spring-integration-4.0.xsd

+						</xsd:documentation>
+					</xsd:annotation>
+				</xsd:attribute>
+				<xsd:attribute name="release-strategy-scheduler" type="xsd:string" default="taskScheduler">


Just call this scheduler, like in delayer.

JIRA: https://jira.spring.io/browse/INT-2595 * Add `group-timeout` and `group-timeout-expression` to the Correlation Endpoint * Add logic to the `AbstractCorrelatingMessageHandler` to schedule group for `forceComplete`, when the target `ReleaseStrategy` returns `false`

artembilan · 2014-04-03T10:41:09Z

Pushed.
Second attempt

garyrussell · 2014-04-03T13:19:38Z

.../main/java/org/springframework/integration/aggregator/AbstractCorrelatingMessageHandler.java

@@ -294,6 +325,13 @@ protected void handleMessageInternal(Message<?> message) throws Exception {
 				messageGroup = this.store(correlationKey, message);

 				if (releaseStrategy.canRelease(messageGroup)) {
+					ScheduledFuture<?> scheduledFuture = this.expireGroupScheduledFutures.get(groupIdUuid);


We need to cancel the schedule regardless of whether we can release or not; we should do it as soon as possible after we get the lock, to minimize the possibility of interrupting the scheduled thread.

artembilan · 2014-04-03T13:28:57Z

Cool! Or comments are good and reasonable. Will be applied

garyrussell · 2014-04-03T13:29:47Z

.../main/java/org/springframework/integration/aggregator/AbstractCorrelatingMessageHandler.java

@@ -166,6 +182,20 @@ public void setOutputChannelName(String outputChannelName) {
 		this.outputChannelName = outputChannelName;
 	}

+	public void setGroupTimeoutExpression(Expression groupTimeoutExpression) {


Wouldn't an overloaded method that takes a String be easier for DSL, and @Bean config?

In DSL it will be populated from: https://github.com/spring-projects/spring-integration-extensions/blob/master/spring-integration-java-dsl/src/main/java/org/springframework/integration/dsl/CorrelationHandlerSpec.java via methods groupTimeout(long value) and groupTimeoutExpression(String expression) and parsed internaly to the Expression.

artembilan · 2014-04-03T16:03:30Z

Pushed

garyrussell · 2014-04-03T21:43:38Z

LGTM; I will do some testing tomorrow.

We need to explain the relationship between this and the reaper; maybe the reaper can be deprecated ? On the other hand, maybe some people will still require the expiration to happen on a schedule rather than a separate schedule for each group.

We should also explain again in the docs that what happens when the group times out depends on sendPartialResultOnExpiry.

I wonder if we should add some logic to also allow removal of empty groups - i.e. schedule another forceComplete() (using now + minimumTimeoutForEmptyGroups) when a group is released but expire-empty-groups-upon-completion is false ?

Or, they can continue to use the reaper for removing empty groups.

artembilan · 2014-04-04T09:08:10Z

Pushed.

Don't see reason to deprecate Reaper and also don't see reason to remove empty groups by their own shedule - it is the Reaper responsibility.

garyrussell · 2014-04-04T20:13:15Z

LGTM; a little doc and test polish here... garyrussell@11b0cfd

artembilan · 2014-04-05T08:40:57Z

Thanks, Gary. I applied your polishing - continue to study english 😄.
However I've pushed some further minor polishing

garyrussell · 2014-04-05T14:27:12Z

Your English is amazing!

Just a little more polish here garyrussell@7fec667

and then I think I can merge.

artembilan · 2014-04-05T14:40:34Z

Polishing - LGTM.
After merge your Lock PR should be rebased and has to provide at least one integration test with this new lock-registry attribute.

garyrussell · 2014-04-05T14:52:52Z

Agree - will do.

garyrussell · 2014-04-05T14:56:34Z

Merged

garyrussell reviewed Apr 2, 2014
View reviewed changes

artembilan changed the title ~~INT-2595: Add ReaperReleaseStrategy~~ INT-2595: Add time-based ReleaseStrategy option Apr 3, 2014

garyrussell reviewed Apr 3, 2014
View reviewed changes

INT-2595: Polishing according PR comments

8e3fb42

INT-2595: Expose lock-registry and further docs

01d7167

INT-2595 Doc and Test Polishing

11b0cfd

INT-2595: Fix typos

3885570

garyrussell closed this Apr 5, 2014

artembilan deleted the INT-2595 branch April 25, 2014 14:56

spring-operator mentioned this pull request May 27, 2014

Time-based ReleaseStrategy [INT-2595] #6575

Closed

INT-2595: Add time-based ReleaseStrategy option #1100

INT-2595: Add time-based ReleaseStrategy option #1100

Uh oh!

Conversation

artembilan commented Apr 2, 2014

Uh oh!

artembilan commented Apr 2, 2014

Uh oh!

artembilan commented Apr 2, 2014

Uh oh!

garyrussell commented Apr 2, 2014

Uh oh!

garyrussell Apr 2, 2014

Choose a reason for hiding this comment

Uh oh!

artembilan Apr 2, 2014

Choose a reason for hiding this comment

Uh oh!

garyrussell commented Apr 2, 2014

Uh oh!

garyrussell Apr 2, 2014

Choose a reason for hiding this comment

Uh oh!

artembilan commented Apr 3, 2014

Uh oh!

garyrussell Apr 3, 2014

Choose a reason for hiding this comment

Uh oh!

artembilan commented Apr 3, 2014

Uh oh!

garyrussell Apr 3, 2014

Choose a reason for hiding this comment

Uh oh!

artembilan Apr 3, 2014

Choose a reason for hiding this comment

Uh oh!

artembilan commented Apr 3, 2014

Uh oh!

garyrussell commented Apr 3, 2014

Uh oh!

artembilan commented Apr 4, 2014

Uh oh!

garyrussell commented Apr 4, 2014

Uh oh!

artembilan commented Apr 5, 2014

Uh oh!

garyrussell commented Apr 5, 2014

Uh oh!

artembilan commented Apr 5, 2014

Uh oh!

garyrussell commented Apr 5, 2014

Uh oh!

garyrussell commented Apr 5, 2014

Uh oh!

Uh oh!

INT-2595: Add time-based `ReleaseStrategy` option #1100

INT-2595: Add time-based `ReleaseStrategy` option #1100