Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(bridge): support incoming mosquitto bridge connections #584
feat(bridge): support incoming mosquitto bridge connections #584
Changes from 5 commits
ce460b8
4b5ffaa
dc586d2
45b4f0c
b4ad77d
1c1e1f2
5e593e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why? This is used to prevent other broker instances to store this packet again when it's emiitted
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.
Unfortunately this cannot be easily removed.
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.
We need to keep the retain on the packet until we are able to check if "retain as published" (rap) is true. That's the whole point of the rap flag. The bridge protocol enables retain-as-published. I moved this into the subscription handler. What are the concerns with moving this into the subscription handler?
The tests are passing with my change, so if there is a problem with removing this line, it must not have been covered by the tests. I'm happy to make adjustments, but I'd like to know more about the concerns.
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.
can you link it to the block of code that replaces this?
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.
@mcollina this is what handlePacketSubscription in subscribe.js is doing. Instead of globally setting it to false for all subscriptions, it sets it to false only if the retain-as-published flag is not set on the subscription.
If clusters go through a different (untested?) path, then clearing the retain flag can happen on that path, or I could set up a different emitter for client subscriptions so that clusters always get the flag set to false while bridges and regular clients respect the rap flag.
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 think we should verify if this is still needed, e.g. if the packet gets stored multiple times when using a cluster.
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.
This only happens when a user sends a packet using
broker.publish
function. BTW I think that it could be safely removed by double checking the code, the 'worst' thing that could happen is some useless write to the dbThere 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 had thought of the
_stored
idea, but wasn't sure whether it would cause other issues. In any case, it becomes the event listener's responsibility to not retain the packet if it's not supposed to. I guess that can be a breaking change for some custom code, so I understand it's a bit dangerous. But I don't see a good way to support bridge protocol (or, going forward, MQTT v5) without doing this.As I've said, the tests pass, including some new ones I added, so how can I verify this concern?
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.
You should add a test where you do a broker subscribe followed by a broker publish to check if the received message has the retained flag cleared or not
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 added the test and, indeed, the retain flag was previously set to false and is now true. I've pushed a change with a proposed solution. The publish function now takes the
rap
parameter and applies the wrapper function that I had previously placed insubscription.js
. This way all callers ofsubscribe
have the retain-as-published functionality available to them but default to the standard behaviour of settingretain
tofalse
.