[#109]Implement Correlation metadata on Sender OutboundMessages #110

Sage-Pierce · 2019-09-24T20:28:40Z

Still a work-in-progress, but looking for feedback. Trying to consider an approach that isn't technically backward incompatible due to the now-necessary generic type declaration on the Flux's OutboundMessage correlation metadata type.

Sage-Pierce · 2019-09-25T19:04:49Z

src/main/java/reactor/rabbitmq/Sender.java

@@ -207,19 +207,61 @@ protected Scheduler createScheduler(String name) {
 * @see SendOptions#trackReturned(boolean)
 */
 public Flux<OutboundMessageResult> sendWithPublishConfirms(Publisher<OutboundMessage> messages, SendOptions options) {
+ return sendWithCorrelatedPublishConfirms(Flux.from(messages).map(message -> (OutboundMessage<Object>) message), options)
+ .map(Function.identity());


This is the only part of the changes that are somewhat squint-worthy to me, but necessary to make the compiler happy

the suggestions above will remove the need for such casts, but you'll probably need 2 separate operator implementations, or at least a bit more juggling at the operator level

simonbasle

I'd try not to force correlationMetadata on current users that don't need it

simonbasle · 2019-09-26T10:22:56Z

src/main/java/reactor/rabbitmq/OutboundMessage.java

@@ -23,7 +23,7 @@
 /**
 * Outbound message meant to be sent by a {@link Sender}.
 */
-public class OutboundMessage {
+public class OutboundMessage<T> {


rather than force a null correlationMetadata on every user, I'd extend OutboundMessage and put the generic and the field on the subclass (OutboundCorrelatedMessage?)

simonbasle · 2019-09-26T10:24:43Z

src/main/java/reactor/rabbitmq/OutboundMessageResult.java

@@ -19,9 +19,9 @@
 /**
 * Result of a sent message when using publisher confirm.
 */
-public class OutboundMessageResult {
+public class OutboundMessageResult<T> {


similarly to the above comment, I'd extract this specificity in a OutboundCorrelatedMessageResult<T>. Then Sender can have methods that take a publisher of OutboundMessage and return Flux<OutboundMessageResult vs methods that take a publisher of OutboundCorrelatedMessage<T> and return Flux<OutboundCorrelatedMessageResult<T>>

simonbasle · 2019-09-26T10:25:02Z

src/main/java/reactor/rabbitmq/OutboundMessage.java

+ public T getCorrelationMetadata() {
+ return correlationMetadata;
+ }
+
 @Override
 public String toString() {


note: toString should probably reflect new field?

simonbasle · 2019-09-26T10:25:39Z

src/main/java/reactor/rabbitmq/Sender.java

@@ -207,19 +207,61 @@ protected Scheduler createScheduler(String name) {
 * @see SendOptions#trackReturned(boolean)
 */
 public Flux<OutboundMessageResult> sendWithPublishConfirms(Publisher<OutboundMessage> messages, SendOptions options) {
+ return sendWithCorrelatedPublishConfirms(Flux.from(messages).map(message -> (OutboundMessage<Object>) message), options)
+ .map(Function.identity());


the suggestions above will remove the need for such casts, but you'll probably need 2 separate operator implementations, or at least a bit more juggling at the operator level

acogoluegnes

This new version integrates much smoother than the first one. I agree with @simonbasle regarding the introduction of 2 sub-classes (e.g. CorrelableOutboundMessage and CorrelableOutboundMessageResult) to make this new feature an opt-in. Users won't carry the empty correlation metadata and won't use a raw type if they don't want to use any correlation. The tricky part will be the operator, but handling the 2 types in there should be doable without too much duplication.

Sage-Pierce · 2019-09-26T17:08:09Z

@acogoluegnes @simonbasle Thank you both for the callouts and suggestions 👍 I'll refactor to attempt addressing your comments

Sage-Pierce · 2019-09-26T22:20:47Z

@acogoluegnes @simonbasle Your recommendations ended up helping me clean these changes up a bit. The main thing that needed generification was the type of OutboundMessage. Once that was generified, nothing outside of type compilation issues needed to be addressed, and I was able to reuse the existing PublishConfirmOperator as-is. I still needed to add new methods for "typed" sends in order to avoid a backward incompatible change (returning Flux<OutboundMessageResult> vs. Flux<OutboundMessageResult<T>>). I believe that makes this changeset now fully backward compatible

Sage-Pierce · 2019-09-26T22:27:06Z

The API could be made a bit more strict and restrict users from being able to use their own extensions of OutboundMessage, and only allow OutboundMessage or CorrelableOutboundMessage, but once I realized this approach worked, figured it would be worthwhile to get your opinions on it

Sage-Pierce · 2019-09-26T22:44:38Z

src/main/java/reactor/rabbitmq/Sender.java

@@ -207,19 +207,60 @@ protected Scheduler createScheduler(String name) {
 * @see SendOptions#trackReturned(boolean)
 */
 public Flux<OutboundMessageResult> sendWithPublishConfirms(Publisher<OutboundMessage> messages, SendOptions options) {
+ return Flux.from(sendWithTypedPublishConfirms(messages, options));


No longer need the weird type casting here

Sage-Pierce · 2019-09-26T22:45:52Z

src/main/java/reactor/rabbitmq/OutboundMessageResult.java


- private final OutboundMessage outboundMessage;
+ private final T outboundMessage;


Users that don't use special OutboundMessage extensions should still be able to use untyped/raw OutboundMessageResult in backward compatible fashion

simonbasle

looks far better to me 👍
maybe I would rename the <T> type to something that reflects that there is a type constraint, like <OMSG>, but that's all

acogoluegnes

Great, a new feature without breaking anything! My only concern is that people using sendWithPublishConfirms will use raw OutboundMessageResults, but this is acceptable.

I think the plan would be for sendWithPublishConfirms to adopt the same signature as sendWithTypedPublishConfirms in 2.0, and then make sendWithTypedPublishConfirms deprecated, to be removed in 3.0. This would be a breaking change for anyone declaring a variable out of the flux of OutboundMessageResult, unfortunate, but this is what major releases are for.

Sage-Pierce · 2019-09-27T15:12:34Z

Thanks for the quick feedback and merge turnaround, @acogoluegnes and @simonbasle !

Is there anywhere I can monitor the release timeline for a new version that will have this update in it? Looks like 1.4.0?

acogoluegnes · 2019-09-30T07:17:45Z

This feature will indeed be in 1.4, which should be released in a few weeks. It should contain #111 as well and anything related to feedback from 1.3.

[#109]Implement Correlation metadata on Sender OutboundMessages

607aee3

Sage-Pierce commented Sep 25, 2019

View reviewed changes

acogoluegnes requested review from simonbasle and bsideup September 26, 2019 08:06

simonbasle requested changes Sep 26, 2019

View reviewed changes

acogoluegnes requested changes Sep 26, 2019

View reviewed changes

[#109]Revise approach for backward compatibility

e834605

[#109]Refactor Generic typing to be in terms of Message and not metadata

e28d855

Sage-Pierce commented Sep 26, 2019

View reviewed changes

acogoluegnes requested a review from simonbasle September 27, 2019 07:20

simonbasle approved these changes Sep 27, 2019

View reviewed changes

acogoluegnes approved these changes Sep 27, 2019

View reviewed changes

acogoluegnes merged commit 48d3363 into reactor:master Sep 27, 2019

acogoluegnes mentioned this pull request Sep 30, 2019

Enable CorrelationMetadata Emission on SenderMessageResults, Similar to Reactor-Kafka #109

Closed

acogoluegnes mentioned this pull request Sep 30, 2019

Make Sender#sendWithPublishConfirms use generics #112

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#109]Implement Correlation metadata on Sender OutboundMessages #110

[#109]Implement Correlation metadata on Sender OutboundMessages #110

Sage-Pierce commented Sep 24, 2019

Sage-Pierce Sep 25, 2019 •

edited

Loading

simonbasle Sep 26, 2019

simonbasle left a comment

simonbasle Sep 26, 2019

simonbasle Sep 26, 2019

simonbasle Sep 26, 2019

simonbasle Sep 26, 2019

acogoluegnes left a comment

Sage-Pierce commented Sep 26, 2019

Sage-Pierce commented Sep 26, 2019 •

edited

Loading

Sage-Pierce commented Sep 26, 2019

Sage-Pierce Sep 26, 2019

Sage-Pierce Sep 26, 2019

simonbasle left a comment

acogoluegnes left a comment

Sage-Pierce commented Sep 27, 2019

acogoluegnes commented Sep 30, 2019


		private final OutboundMessage outboundMessage;
		private final T outboundMessage;

[#109]Implement Correlation metadata on Sender OutboundMessages #110

[#109]Implement Correlation metadata on Sender OutboundMessages #110

Conversation

Sage-Pierce commented Sep 24, 2019

Sage-Pierce Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

simonbasle Sep 26, 2019

Choose a reason for hiding this comment

simonbasle left a comment

Choose a reason for hiding this comment

simonbasle Sep 26, 2019

Choose a reason for hiding this comment

simonbasle Sep 26, 2019

Choose a reason for hiding this comment

simonbasle Sep 26, 2019

Choose a reason for hiding this comment

simonbasle Sep 26, 2019

Choose a reason for hiding this comment

acogoluegnes left a comment

Choose a reason for hiding this comment

Sage-Pierce commented Sep 26, 2019

Sage-Pierce commented Sep 26, 2019 • edited Loading

Sage-Pierce commented Sep 26, 2019

Sage-Pierce Sep 26, 2019

Choose a reason for hiding this comment

Sage-Pierce Sep 26, 2019

Choose a reason for hiding this comment

simonbasle left a comment

Choose a reason for hiding this comment

acogoluegnes left a comment

Choose a reason for hiding this comment

Sage-Pierce commented Sep 27, 2019

acogoluegnes commented Sep 30, 2019

Sage-Pierce Sep 25, 2019 •

edited

Loading

Sage-Pierce commented Sep 26, 2019 •

edited

Loading