fix: manually republish messages on built-in bridge reconnect#3717
fix: manually republish messages on built-in bridge reconnect#3717jarhodes314 merged 1 commit intothin-edge:mainfrom
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
9954874 to
fe29324
Compare
fe29324 to
f65270e
Compare
There was a problem hiding this comment.
This file is a mod.rs so it doesn't get compiled into its own test binary. It will be compiled as a submodule of republish.rs so the tests in this file will run as part of this.
The first draft of this code was created by gemini, hence the heavy commenting. I intend to remove some of the less useful ones, although the details of what the broker should be expecting are broadly useful.
Robot Results
|
didier-wenzek
left a comment
There was a problem hiding this comment.
I really like these new tests driven by small precise actions on the local and remote brokers.
The code of the test_broker used underneath is a bit weird but that's okay as the method to control its behavior from the test suite are clear enough.
I will be happy to approve this PR once the clippy warnings fixed (or simply marked as ignored, since they are related to the test_broker).
a169b70 to
5f00c08
Compare
albinsuresh
left a comment
There was a problem hiding this comment.
Just some queries to better understand some aspects of the fix.
| let mut published = 0; // Count of messages published (by the companion) | ||
| let mut acknowledged = 0; // Count of messages acknowledged (by the MQTT end-point of the companion) | ||
|
|
||
| let mut session_present: Option<bool> = None; |
There was a problem hiding this comment.
I was wondering why this is not initiated by the clean_session value used in the mqtt config used to establish the connection. Got the answer from the code below that it is decided based on the broker response, since some brokers like C8Y can ignore the client's request for a persistent session. A comment would be helpful for future readers.
There was a problem hiding this comment.
Now added a comment
| // We have to subscribe to this asynchronously (i.e. in a task) since we might at | ||
| // this point have filled our cloud event loop with outgoing messages | ||
| tokio::spawn(async move { recv_client.subscribe_many(topics).await.unwrap() }); | ||
| { |
There was a problem hiding this comment.
Why was this code moved into a sub-block? To drop the recv_client handle as quickly as possible? Any subtle impact on the fix with this change?
There was a problem hiding this comment.
Ah, I think that's a relic of the initial implementation where I attempted to resend the messages manually and needed multiple clones of the client and topics, so I put this into a block to allow me to reuse names.
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
8048e66 to
14136b4
Compare
Proposed changes
Manually republish messages on bridge reconnect to simulate non-clean sessions for Cumulocity.
Since v0.24, rumqttc has fixed how it handles reconnection to a broker, only republishing the existing packets when a session is present (i.e. we have connected to a broker with
clean_session=falseand the broker has acknowledged this in the connack packet). For Cumulocity, we ideally want a non-clean (i.e. persistent) session, but this is explicitly recommended against.Still TODO:
Types of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments