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

:fix: add auto reconnect to service client #4012

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

riccardomodanese
Copy link
Contributor

Brief description of the PR.
Added a connection retry when the conection is dropped.

Related Issue
fixes 4011

Description of the solution adopted
Like for the service event connection a connection retry was added. This is triggered, through the connection event listener, when the connection is dropped.

Screenshots
none

Any side note on the changes made
none

@riccardomodanese riccardomodanese added the Bug This is a bug or an unexpected behaviour. Fix it! label Apr 5, 2024
@riccardomodanese riccardomodanese force-pushed the fix-service_client_auto_reconnect branch from 5ae4ffa to f2243bc Compare April 5, 2024 14:31
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 58 lines in your changes are missing coverage. Please review.

Project coverage is 16.86%. Comparing base (81a090d) to head (f2243bc).
Report is 6 commits behind head on develop.

❗ Current head f2243bc differs from pull request most recent head fc8ef49. Consider uploading reports for the commit fc8ef49 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #4012      +/-   ##
=============================================
- Coverage      16.87%   16.86%   -0.01%     
  Complexity        10       10              
=============================================
  Files           1989     1987       -2     
  Lines          51791    51810      +19     
  Branches        4420     4421       +1     
=============================================
  Hits            8739     8739              
- Misses         42648    42667      +19     
  Partials         404      404              
Files Coverage Δ
...ua/client/security/ServiceClientMessagingImpl.java 0.00% <ø> (ø)
...se/kapua/commons/event/jms/JMSServiceEventBus.java 19.21% <0.00%> (ø)
...broker/artemis/plugin/security/SecurityPlugin.java 0.00% <0.00%> (ø)
...lipse/kapua/client/security/amqpclient/Client.java 0.00% <0.00%> (ø)

Copy link
Contributor

@elbert3 elbert3 left a comment

Choose a reason for hiding this comment

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

Hi @riccardomodanese, I added a few questions.

consumer.setMessageListener(clientMessageListener);
producer = session.createProducer(session.createQueue(requestAddress));
clientMessageListener.init(session, producer);
connectionStatus = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there not a way to read the connection status from one of these objects instead of managing it ourselves? Should it be set to false on exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no. The javax.jms.Connection has no connect flag o isConnect method.


private void disconnect() throws JMSException {
if (connection != null) {
connection.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we set the connectionStatus here to false?

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 was relying on the ExceptionListener but you are right: it's better to set it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay let's add it in, then I'd be okay approving this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@riccardomodanese riccardomodanese force-pushed the fix-service_client_auto_reconnect branch 2 times, most recently from bb46a1d to 979de0d Compare April 10, 2024 12:33
@riccardomodanese riccardomodanese force-pushed the fix-service_client_auto_reconnect branch from 979de0d to dbe3bb8 Compare April 11, 2024 12:05
Signed-off-by: riccardomodanese <riccardo.modanese@eurotech.com>
@riccardomodanese riccardomodanese force-pushed the fix-service_client_auto_reconnect branch from dbe3bb8 to fc8ef49 Compare April 11, 2024 12:40
@Coduz Coduz merged commit b073aaa into develop Apr 11, 2024
65 checks passed
@Coduz Coduz deleted the fix-service_client_auto_reconnect branch July 11, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This is a bug or an unexpected behaviour. Fix it!
Projects
Development

Successfully merging this pull request may close these issues.

3 participants