Skip to content

feat: add MQTT keepalive_interval config to allow custom value per bridge#3365

Merged
rina23q merged 1 commit intothin-edge:mainfrom
rina23q:feature/3153/configurable-mqtt-keepalive-interval-to-cloud-bridge
Jan 29, 2025
Merged

feat: add MQTT keepalive_interval config to allow custom value per bridge#3365
rina23q merged 1 commit intothin-edge:mainfrom
rina23q:feature/3153/configurable-mqtt-keepalive-interval-to-cloud-bridge

Conversation

@rina23q
Copy link
Copy Markdown
Member

@rina23q rina23q commented Jan 27, 2025 •
edited
Loading

Proposed changes

As per the request in #3153, add <cloud>.bridge.keepalive_interval to the tedge config settings.
Example usage:

tedge config set c8y.bridge.keepalive_interval 60s

With mosquitto, the value will be shown explicitly in c8y-bridge.conf as :

### Bridge
....
keepalive_interval 60

Using 60s as default as I found 60 seconds is the default for both mosquitto and rumqttc.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3153

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@rina23q rina23q temporarily deployed to Test Pull Request January 27, 2025 18:59 — with GitHub Actions Inactive
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 27, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 69.38776% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
.../tedge_config/src/tedge_config_cli/tedge_config.rs 23.07% 0 Missing and 10 partials ⚠️
crates/core/tedge/src/bridge/config.rs 92.85% 0 Missing and 1 partial ⚠️
crates/core/tedge/src/cli/connect/command.rs 0.00% 0 Missing and 1 partial ⚠️
crates/core/tedge_mapper/src/aws/mapper.rs 0.00% 1 Missing ⚠️
crates/core/tedge_mapper/src/az/mapper.rs 0.00% 1 Missing ⚠️
crates/core/tedge_mapper/src/c8y/mapper.rs 0.00% 1 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

albinsuresh
albinsuresh reviewed Jan 28, 2025
View reviewed changes
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 28, 2025 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
556 0 2 556 100 1h29m27.124075s

@rina23q rina23q temporarily deployed to Test Pull Request January 28, 2025 12:17 — with GitHub Actions Inactive
@rina23q rina23q changed the title Add MQTT keepalive_interval config to allow custom value per bridge feat: add MQTT keepalive_interval config to allow custom value per bridge Jan 28, 2025
didier-wenzek
didier-wenzek approved these changes Jan 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved.

Unrelated to this PR:

  • The changes for c8y, az and aws bridges are so similar that it sounds as a failure to be DRY. Actually, the main point is to configure a mosquitto bridge and one should have a mosquitto::bridge abstraction, not an abstraction per cloud.

albinsuresh
albinsuresh approved these changes Jan 29, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

LGTM

albinsuresh
albinsuresh reviewed Jan 29, 2025
View reviewed changes
crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

/// The amount of time after which the bridge should send a ping if no other traffic has occured
#[tedge_config(example = "60s", default(from_str = "60s"))]
keepalive_interval: SecondsOrHumanTime,
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh Jan 29, 2025

Choose a reason for hiding this comment

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

Optional:

Suggested change
keepalive_interval: SecondsOrHumanTime,
keep_alive_interval: SecondsOrHumanTime,

Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I take that back, after noticing that the term keepalive is kinda like the norm across protocols like HTTP and GRPC.

@rina23q
Add MQTT keepalive_interval config to allow custom value per bridge
2a5a2ec 
Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
@rina23q rina23q force-pushed the feature/3153/configurable-mqtt-keepalive-interval-to-cloud-bridge branch from 45bf3d9 to 2a5a2ec Compare January 29, 2025 12:34
@rina23q rina23q temporarily deployed to Test Pull Request January 29, 2025 12:35 — with GitHub Actions Inactive
@rina23q rina23q added theme:mqtt Theme: mqtt and mosquitto related topics theme:configuration Theme: Configuration management theme:bridge Built-in (Rust) bridge topics labels Jan 29, 2025
@rina23q rina23q added this pull request to the merge queue Jan 29, 2025
Merged via the queue into thin-edge:main with commit ce649ba Jan 29, 2025
@rina23q rina23q deleted the feature/3153/configurable-mqtt-keepalive-interval-to-cloud-bridge branch January 29, 2025 13:42
@rina23q rina23q mentioned this pull request Jan 29, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albinsuresh albinsuresh albinsuresh approved these changes

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

Assignees

No one assigned

Labels

theme:bridge Built-in (Rust) bridge topics theme:configuration Theme: Configuration management theme:mqtt Theme: mqtt and mosquitto related topics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rina23q @albinsuresh @didier-wenzek