Skip to content

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

Closed
artembilan wants to merge 5 commits into
spring-projects:masterfrom
artembilan:INT-2595
Closed

INT-2595: Add time-based ReleaseStrategy option#1100
artembilan wants to merge 5 commits into
spring-projects:masterfrom
artembilan:INT-2595

Conversation

@artembilan
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member Author

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

@artembilan
Copy link
Copy Markdown
Member Author

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

@garyrussell
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member Author

Pushed.
Second attempt

@artembilan artembilan changed the title INT-2595: Add ReaperReleaseStrategy INT-2595: Add time-based ReleaseStrategy option Apr 3, 2014
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member Author

Pushed

@garyrussell
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Contributor

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

@artembilan
Copy link
Copy Markdown
Member Author

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

@garyrussell
Copy link
Copy Markdown
Contributor

Your English is amazing!

Just a little more polish here garyrussell@7fec667

and then I think I can merge.

@artembilan
Copy link
Copy Markdown
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants