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

BrokerAvailabilityEvent without any relay node information? #25890

Closed
rupebac opened this issue Oct 9, 2020 · 6 comments
Closed

BrokerAvailabilityEvent without any relay node information? #25890

rupebac opened this issue Oct 9, 2020 · 6 comments
Labels
in: messaging Issues in messaging modules (jms, messaging)

Comments

@rupebac
Copy link

rupebac commented Oct 9, 2020

Scenario: I am connecting to a cluster of Stomp relays (without external LB), one of the Stomp nodes goes offline and I therefore receive the right BrokerAvailabilityEvent, but from this, I can't see how to grab the specific TcpConnectionHandler instance that triggered the event. I mean, it is hidden. Without that, how am I supposed to know which node exactly went offline? My intention was to leave this node out of the candidates pool for some time (5min or so) before consider putting him back to make him a candidate on the tcpClient#remoteAddress().

The whole thing I think is misconceived, it seems to be taking the relay port just from the StompBrokerRelayRegistration, without considering it may be chosen dynamically by the TcpClient layer underneath.

Am I missing something?

@rupebac rupebac changed the title StompBrokerRelayMessage with wrong StompBrokerRelayMessage BrokerAvailabilityEvent without any relay node information? Oct 9, 2020
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 9, 2020
@rstoyanchev rstoyanchev added the in: messaging Issues in messaging modules (jms, messaging) label Oct 16, 2020
@rstoyanchev
Copy link
Contributor

StompBrokerRelayMessageHandler has no knowledge of the remote address(es) and TcpConnectionHandler is just a callback interface that doesn't expose any information, so I don't see how adding that onto the event would help.

I am wondering have you considered using TcpClient callbacks like doOnDisconnected? That would help you to detect when a connection is lost and you could perhaps use that to decide which ones to leave out for a certain period?

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Oct 16, 2020
@rupebac
Copy link
Author

rupebac commented Oct 16, 2020

Thank you very much, yes, I ended up doing what you are saying. However it is not as simple as that, since doOnDisconnected is also triggered when the user closes the session gently by sending DISCONNECT. I have managed to work around by adding an OutboundAdapter that listens on "close" (which is triggered when forwarding a DISCONNECT), and adding an InboundAdapter that listens on channelInactive. In all my tests this is working as a charm. Why can't you do that and trigger BrokerAvailabilityEvent from there? If you are open for a PR, I can submit.

This simple thing took me quite a bit of time to achieve, and it woud be great if BrokerAvailabilityEvent is not only bound and triggered from the system session, but also when the user session is being closed unexpectedly from the broker side right?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 16, 2020
@rstoyanchev
Copy link
Contributor

Again I don't see any way for the broker to obtain this information. It's just working with TcpOperations.

@rupebac
Copy link
Author

rupebac commented Oct 16, 2020

Why are you not triggering the event here for example:

throw new MessageDeliveryException("Message broker not active. Consider subscribing to " +

?

If you only trigger it from the system session, then I guess it is relatively easy to know to which server the system session is connected (specially if you would approve my other PR in #25889).

@rstoyanchev
Copy link
Contributor

There is no need to trigger the event there as well. That's for messages specifically to the system session which already detects publishes the events.

I guess it is relatively easy to know to which server the system session is connected

Only if you use the relayHost and relayPort properties but in that case there isn't much to guess. If you configure TcpClient directly, the StompBrokerMessageHandler does not know the remote address.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 19, 2020
@rupebac
Copy link
Author

rupebac commented Oct 20, 2020

That's for messages specifically to the system session which already detects publishes the events.

I know this is triggered from handleMessages which should obviously not be triggered if connectivity is broken. But I have seen these exceptions, even before any BrokerAvailabilityEvent arrives. It is hard for me now to give more details, since I do not know how I came across that. Maybe from a user session sendStompErrorFrameToClient -> handleInboundMessage(...)? Anyway, I will provide more details if I ever come across this issue again, now the infrastructure from my side is safer and proactive and harder to run in these issues.

Also the solution you gave for me other issue #25889 is helping me to find out where the system session is connected, and this is not necessary anymore as long as you are only triggering this event from SystemStompConnectionHandler.

Due to all this, I close this issue. Feel free to contact me for further help/review.

Thanks a lot

@rupebac rupebac closed this as completed Oct 20, 2020
@rstoyanchev rstoyanchev removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: messaging Issues in messaging modules (jms, messaging)
Projects
None yet
Development

No branches or pull requests

3 participants