-
Notifications
You must be signed in to change notification settings - Fork 55
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: increase max packet size for built-in bridge #3059
fix: increase max packet size for built-in bridge #3059
Conversation
Signed-off-by: James Rhodes <[email protected]>
Signed-off-by: James Rhodes <[email protected]>
Signed-off-by: James Rhodes <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
Robot Results
|
@@ -57,6 +57,8 @@ use crate::topics::matches_ignore_dollar_prefix; | |||
use crate::topics::TopicConverter; | |||
pub use config::*; | |||
|
|||
const MAX_PACKET_SIZE: usize = 10 * 1024 * 1024; |
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.
Looks like we have some inconsistency with packet sizes used across the project.
- In
mqtt_channel::config::Config
: 1024 * 1024 - In
bridge::common_mosquitto_config::CommonMosquittoConfig
: 256 * 1024 * 1024 - 1 - In
tedge::cli::mqtt::MAX_PACKET_SIZE
: 10 * 1024 * 1024
Any reason why you chose the 3rd option and not the second one, keeping it consistent with the existing bridge config?
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 didn't realise that mosquitto used a completely different name for the configuration, so didn't notice it. I'll change this to be consistent with the existing bridge.
if pkid != 0 { | ||
// Messages with pkid 0 (meaning QoS=0) should not be added to the hashmap | ||
// as multiple messages with the pkid=0 can be received | ||
e.insert(msg); | ||
} |
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 this pkid = 0
case be covered with a unit test?
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.
Not easily. In an ideal world, we would have some deterministic tests where we control the broker we connect to directly, so we can send QoS 0 messages. This would also shore up the existing integration tests, which have to flood the bridge with thousands of messages before disconnecting to reproduce the desired behaviour.
I think this might be worth investigating in a time-boxed manner, but I'll create a separate ticket for it.
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 this
pkid = 0
case be covered with a unit test?
This is tested by the system test "Support publishing QoS 0 messages to c8y topic #2960".
Sure, this test is not deterministic (as messages with QoS 0 might not be delivered), but I think that is enough.
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.
LGTM
Signed-off-by: James Rhodes <[email protected]>
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.
Approved
Proposed changes
tedge mqtt pub
Types of changes
Paste Link to the issue
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments