INT-4542: CorrMH: Use popSequenceDetails properly #2591

artembilan · 2018-10-08T20:33:28Z

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

Perform a MessageBuilder.popSequenceDetails() for those
MessageGroupProcessor results which are not Message
or Collection<Message>

garyrussell · 2018-10-08T20:36:08Z

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

@@ -653,7 +654,8 @@ protected void forceComplete(MessageGroup group) {
 						 * groups. A longer timeout for empty groups can be enabled by
 						 * setting minimumTimeoutForEmptyGroups.
 						 */
-						removeGroup = lastModifiedNow <= (System.currentTimeMillis() - this.minimumTimeoutForEmptyGroups);
+						removeGroup = lastModifiedNow <= (System
+								.currentTimeMillis() - this.minimumTimeoutForEmptyGroups);


This looks odd; move System to the next line too?

Hm. Some fault of my IDEA code reformatting.
Will fix.

Still weird indentation.

garyrussell · 2018-10-08T20:50:30Z

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

+						.copyHeaders(message.getHeaders());
+			}
+			result = messageBuilder.popSequenceDetails();
+		}


Now I look at this, I wonder if it is completely valid to do this unconditionally. Consider

splitter -> custom-splitter -> custom-aggregator -> aggregator

If the custom splitter doesn't push sequence details, we will remove the real ones from the first splitter.

Perhaps we need a new flag alwaysPopSequenceDetails (true in 5.1, false in 5.0) - even in the abstract output processor.

Or, add some metadata (seqInfoId) to the sequence details and only pop if the splitter and aggregator are configured with the same metadata.

OK. Sounds like we just need to be sure that our aggregation is based on the sequenceDetails. Because custom splitter may not be based on the applySequence.
So, maybe just an extra sequenceApplied header? But then how to understand on which level it has not been used?..

Or... I just don't see the whole picture and need to sleep with this...

After sleeping with this, I came with the conclusion that your suggestion about popSequenceDetails(true|false) is the way to go.
We won't pollute messages with some extra header which may be really out of control and I don't want to tie an aggregator with the splitter.

We may do a condition based on the default HeaderAttributeCorrelationStrategy(IntegrationMessageHeaderAccessor.CORRELATION_ID) and SimpleSequenceSizeReleaseStrategy, but that still doesn't come as a robust solution since we can use defaults, but populate sequence header some other way, not via splitter.

From here I think we just need to add such a popSequenceDetails option to the correlation handler, make it true by default (looks like most cases, since we haven't had issues so far), modify a AbstractAggregatingMessageGroupProcessor to return a AbstractIntegrationMessageBuilder instead of the whole message and do conditional .popSequenceDetails() already in the AbstractCorrelatingMessageHandler.

Saying that I don't think that we are good for back-porting to 5.0.x: looks like this is going to be some breaking change.

Let me know WDYT?
Thanks

Maybe call it simply popSequence with the documentation explaining it undoes the nearest upstream applySequence?

Makes sense. Will fix soon.

JIRA: https://jira.spring.io/browse/INT-4542 Perform a `MessageBuilder.popSequenceDetails()` for those `MessageGroupProcessor` results which are not `Message` or `Collection<Message>` **Cherry-pick to 5.0.x**

garyrussell

Also needs a migration guide indicating that the sequence is now popped by default for all MGPs.

garyrussell · 2018-10-10T17:30:10Z

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

+	 * Perform a {@link MessageBuilder#popSequenceDetails()} for output message or not.
+	 * Default to true.
+	 * This option plays an opposite role to the
+	 * {@link org.springframework.integration.splitter.AbstractMessageSplitter#setApplySequence(boolean)}.


It's not just splitter - router, pubsub etc; maybe just say "removes the sequence information added by the nearest upstream component with applySequence=true (for example splitter).

garyrussell · 2018-10-10T17:30:49Z

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

@@ -653,7 +654,8 @@ protected void forceComplete(MessageGroup group) {
 						 * groups. A longer timeout for empty groups can be enabled by
 						 * setting minimumTimeoutForEmptyGroups.
 						 */
-						removeGroup = lastModifiedNow <= (System.currentTimeMillis() - this.minimumTimeoutForEmptyGroups);
+						removeGroup = lastModifiedNow <= (System
+								.currentTimeMillis() - this.minimumTimeoutForEmptyGroups);


Still weird indentation.

garyrussell · 2018-10-10T17:33:57Z

...-integration-core/src/test/java/org/springframework/integration/dsl/routers/RouterTests.java

+		assertNotNull(bestQuoteMessage);
+		Object payload = bestQuoteMessage.getPayload();
+		assertThat(payload, instanceOf(String.class));
+//		assertThat(((List<?>) payload).size(), greaterThanOrEqualTo(1));


Why is this here? The payload is a String.

garyrussell · 2018-10-10T17:34:45Z

src/reference/asciidoc/aggregator.adoc

@@ -103,6 +103,13 @@ This method is invoked for aggregating messages as follows:

 NOTE: In the interest of code simplicity and promoting best practices such as low coupling, testability, and others, the preferred way of implementing the aggregation logic is through a POJO and using the XML or annotation support for configuring it in the application.

+Starting with version 5.1, after processing message group, an `AbstractCorrelatingMessageHandler` performs a `MessageBuilder.popSequenceDetails()` message headers modification for the proper splitter-aggregator scenario with several nested levels.
+It is done only if the message group release result is not a message or collection of messages.
+In that case a target `MessageGroupProcessor` is responsible for the `MessageBuilder.popSequenceDetails()` call during those messages building.


s/during those messages building/while building those messages/

garyrussell · 2018-10-10T17:35:34Z

src/reference/asciidoc/aggregator.adoc

+Starting with version 5.1, after processing message group, an `AbstractCorrelatingMessageHandler` performs a `MessageBuilder.popSequenceDetails()` message headers modification for the proper splitter-aggregator scenario with several nested levels.
+It is done only if the message group release result is not a message or collection of messages.
+In that case a target `MessageGroupProcessor` is responsible for the `MessageBuilder.popSequenceDetails()` call during those messages building.
+This functionality can be controlled by newly introduced `popSequence` `boolean` property, so the `MessageBuilder.popSequenceDetails()` can be disabled in some scenarios when correlation details have not been populated by the standard splitter.


s/by newly introduced/by a new/

garyrussell · 2018-10-10T17:36:10Z

src/reference/asciidoc/aggregator.adoc

+It is done only if the message group release result is not a message or collection of messages.
+In that case a target `MessageGroupProcessor` is responsible for the `MessageBuilder.popSequenceDetails()` call during those messages building.
+This functionality can be controlled by newly introduced `popSequence` `boolean` property, so the `MessageBuilder.popSequenceDetails()` can be disabled in some scenarios when correlation details have not been populated by the standard splitter.
+This property, essentially, undoes what has been done with the nearest upstream `applySequence = true` in the `AbstractMessageSplitter`.


...by the nearest...

…ce()` * Fix Docs typos and language * finish the `RouterTests.testNestedScatterGather()`

garyrussell · 2018-10-10T18:03:57Z

We're no longer cherry-picking; correct?

artembilan · 2018-10-10T18:07:22Z

We're no longer cherry-picking; correct?

No, we are not. There is a workaround with the custom MessageGroupProcessor and explicit popSequenceDetails() call there.
Plus an AbstractAggregatingMessageGroupProcessor has a breaking change.

Migration Guide note has been added: https://github.com/spring-projects/spring-integration/wiki/Spring-Integration-5.0-to-5.1-Migration-Guide#aggregator-changes

garyrussell suggested changes Oct 8, 2018

View reviewed changes

artembilan force-pushed the INT-4542 branch from 0ae9481 to 393c257 Compare October 10, 2018 14:27

artembilan added 3 commits October 10, 2018 12:56

INT-4542: CorrMH: Use popSequenceDetails properly

ba7405b

JIRA: https://jira.spring.io/browse/INT-4542 Perform a `MessageBuilder.popSequenceDetails()` for those `MessageGroupProcessor` results which are not `Message` or `Collection<Message>` **Cherry-pick to 5.0.x**

* Add AbstractCorrelatingMessageHandler.popSequenceDetails property

f337141

* Rename property to just a popSequence

9d75acf

artembilan force-pushed the INT-4542 branch from 393c257 to 9d75acf Compare October 10, 2018 17:20

garyrussell suggested changes Oct 10, 2018

View reviewed changes

* Fix JavaDocs on the `AbstractCorrelatingMessageHandler.setPopSequen…

81f6e0d

…ce()` * Fix Docs typos and language * finish the `RouterTests.testNestedScatterGather()`

garyrussell approved these changes Oct 10, 2018

View reviewed changes

garyrussell merged commit 0f5cfd4 into spring-projects:master Oct 10, 2018

artembilan mentioned this pull request Jan 27, 2020

Fix for Issue with AbstractCorrelatingMessageHandler in handling sequences when Message is returned from outputProcessor #3160

Closed

INT-4542: CorrMH: Use popSequenceDetails properly #2591

INT-4542: CorrMH: Use popSequenceDetails properly #2591

Uh oh!

Conversation

artembilan commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyrussell Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyrussell commented Oct 10, 2018

Uh oh!

artembilan commented Oct 10, 2018

Uh oh!

Uh oh!

artembilan commented Oct 8, 2018 •

edited

Loading

garyrussell Oct 8, 2018 •

edited

Loading