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

[v6r21] MQ disconnect only producers #4006

Merged
merged 15 commits into from
Apr 14, 2019

Conversation

wkrzemien
Copy link
Collaborator

@wkrzemien wkrzemien commented Mar 27, 2019

Solving part of the issue presented in #3986
When MQ server is restarted, the clients are blocked. This fix add a dedicated listener ReconnectListener which in case of the disconnection tries to reconnect to the server.
The set of options such as number of trials, interval time etc are provided. To reconnect the internal
stomp.py connect mechanism is used.
The current solution solves the producer case. Separate patch will be prepared for the consumer case. This needs more refactorization due to the handling of subscription after the disconnection occurs.

As byproduct, the handling of stomp.exception.NotConnectedException which could be raised inside StompMQConnector subscribe() method, has been added. The exception is handled and it is transformed into a standard S_ERROR.

Integration tests done:

Scenario I:
1.Server is unavailable
2.Producer wants to connect and send a message. Producer will try to reconnect.
3.Server goes up
4.Producer connects and sends the message

Scenario II:
1.Producer is connected.
2.Server goes down. Producer is trying to reconnect
3.Server goes up. Producer connects and can send message.

BEGINRELEASENOTES

*Resources
FIX: Resources/MessageQueue: add a dedicated listener ReconnectListener

ENDRELEASENOTES

Resources/MessageQueue/MQConnector.py Outdated Show resolved Hide resolved
Resources/MessageQueue/StompMQConnector.py Outdated Show resolved Hide resolved
Resources/MessageQueue/StompMQConnector.py Show resolved Hide resolved
self.log.error('Failed to subscribe: %s' % e)
fail = True
if fail:
return S_ERROR(EMQUKN, 'Failed to subscribe to at least one broker')
Copy link
Contributor

Choose a reason for hiding this comment

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

why not returning in the exception directly ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure. I think the argument is the following: this for loop is over all connected brokers (we can have more than one for the same connection), we are tryign to close connection to all brokers. If the error occurs for a given broker should we stop immediatly or try to continue anyway with another brokers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not add this logic, it was like that I just cleaned the code a bit.

@coveralls
Copy link

coveralls commented Mar 27, 2019

Coverage Status

Coverage increased (+0.01%) to 22.183% when pulling 28b4968 on wkrzemien:MQDisconnect_onlyProducer into ba87fcb on DIRACGrid:rel-v6r21.

In connect() method ReconnectListener was added after start() and
connect() method are called. According to internal comments in the stomp.py
and also looking at the examples it seems that listener should be added
before start() method is called.
@wkrzemien wkrzemien changed the title [WIP] MQ disconnect only producers [v6r21] MQ disconnect only producers Mar 30, 2019
@fstagni
Copy link
Contributor

fstagni commented Apr 4, 2019

I don't have specific comments to this PR, so IMHO review OK, but maybe @chaen wants to comment further?

@wkrzemien
Copy link
Collaborator Author

@chaen @fstagni What about this one? Should I change something?

@fstagni
Copy link
Contributor

fstagni commented Apr 9, 2019

For me it's fine.

Copy link
Contributor

@chaen chaen left a comment

Choose a reason for hiding this comment

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

seems ok to me

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