Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Channel mono #42

Merged
merged 5 commits into from Jan 3, 2019
Merged

Channel mono #42

merged 5 commits into from Jan 3, 2019

Conversation

pmackowski
Copy link
Contributor

@pmackowski pmackowski commented Dec 27, 2018

This PR has been triggered by issue#41

There have been added following properties to both SenderOptions and SendOptions:

  • Mono<? extends Channel> channelMono

  • BiConsumer<SignalType, Channel> channelCloseHandler

There is another subtle change referring to discrepancy of caching channel creation

  • Sender#send uses connectionMono.map(CHANNEL_CREATION_FUNCTION).cache()
  • Sender#sendWithPublishConfirms uses connectionMono.map(CHANNEL_CREATION_FUNCTION)

I removed cache() in Sender#send

@pivotal-issuemaster
Copy link

@pmackowski Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@pmackowski Thank you for signing the Contributor License Agreement!

@Override
public void accept(SignalType signalType, Channel channel) {
int channelNumber = channel.getChannelNumber();
LOGGER.info("closing channel {} by signal {}", channelNumber, signalType);

Choose a reason for hiding this comment

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

Are you sure you want to log every channel closure as info? Some environments have high channel churn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to log it as debug. It is extracted from here
channel close

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed logging level to debug

@acogoluegnes
Copy link
Contributor

@pmackowski Thanks for this contribution! Why didn't you add a Mono<Channel in SendOptions to override the one in Sender? Do you think this level of customization isn't necessary?

@pmackowski
Copy link
Contributor Author

@acogoluegnes I did it initially but finally reverted changes in SendOptions revert commit.
The reason was that I could not figure out simple and clean solution without additional changes
e.g. I am not sure how to calculate options in the following method in Sender

public Mono<Void> send(Publisher<OutboundMessage> messages) {
        return send(messages, new SendOptions());
}

Let's take exceptionHandler into account with its default value and suppose it is also defined in SenderOptions. Should we always use the one from SendOptions because it has higher priority or the one from SenderOptions where user defined it and was not aware of SendOptions at all?

If you don't mind I can try to suggest some solution, unless I overlooked something.

@pmackowski pmackowski deleted the channel-mono branch January 3, 2019 12:04
@acogoluegnes
Copy link
Contributor

The one in SendOptions would be null by default so Sender#getChannelMono() would be used by default (and the one from SenderOptions if it's been provided). If a channel mono is to be provided in SendOptions it would be used first. Does that make sense? I think so, and it does not break the existing code.

@pmackowski
Copy link
Contributor Author

Yes, it makes sense with null properties in SendOptions. I will revert reverted commit and create a new PR.

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.

None yet

5 participants