-
Couldn't load subscription status.
- Fork 1.1k
Replace MQTT library #6906
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
Replace MQTT library #6906
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6906 +/- ##
=============================================
- Coverage 72.10% 72.10% -0.01%
+ Complexity 19673 19671 -2
=============================================
Files 2125 2125
Lines 79533 79539 +6
Branches 8051 8051
=============================================
- Hits 57350 57348 -2
- Misses 19346 19354 +8
Partials 2837 2837 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the port issue is resolved, messages seem to be coming through. So in general this should work.
| Mqtt5AsyncClient asyncClient = Mqtt5Client.builder() | ||
| .identifier("OpenTripPlanner-" + UUID.randomUUID()) | ||
| .serverHost(parsedUrl.getHost()) | ||
| .serverPort(parsedUrl.getPort()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this out with the example config. The port is missing in the configuration, which results in URI#getPort returning -1. That leads to this not working. You have to only set the port if it's not equal to -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I added a check for the port.
...rc/main/java/org/opentripplanner/updater/trip/gtfs/updater/mqtt/MqttGtfsRealtimeUpdater.java
Show resolved
Hide resolved
...rc/main/java/org/opentripplanner/updater/trip/gtfs/updater/mqtt/MqttGtfsRealtimeUpdater.java
Show resolved
Hide resolved
|
I tested this in our dev and seems to work as well as the previous library. I can approve this when #6906 (comment) is fixed. |
|
I'm not sure if we should or shouldn't generate a changelog entry from this pr. |
|
All the relevant parties already know about this change so for that reason alone, I decided that the changelog entry isn't needed. |
Summary
This PR replaces the Paho MQTT library with the HiveMQ MQTT library.
Issue
As mentioned in #6851 the Paho MQTT library should be replaced as decided in dev meeting.
Should be tested by HSL.
Unit tests
It is hard to test by unit tests and the change has to be tested by HSL anyway.