Skip to content

fix: clear retained entity twin messages on deregister#3499

Merged
albinsuresh merged 1 commit intothin-edge:mainfrom
albinsuresh:fix/entity-deregister-clear-all-data
Apr 12, 2025
Merged

fix: clear retained entity twin messages on deregister#3499
albinsuresh merged 1 commit intothin-edge:mainfrom
albinsuresh:fix/entity-deregister-clear-all-data

Conversation

@albinsuresh
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh commented Mar 25, 2025

Proposed changes

Clear retained twin data messages on entity deregister.

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

#3492

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
  • I ran just format as mentioned in CODING_GUIDELINES
  • I used just check 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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2025

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
607 0 3 607 100 1h49m30.189813s

@reubenmiller reubenmiller added theme:mqtt Theme: mqtt and mosquitto related topics theme:entity_store Entity store related functionality labels Mar 25, 2025
@albinsuresh albinsuresh force-pushed the fix/entity-deregister-clear-all-data branch from d8f2118 to a51180d Compare March 26, 2025 17:45
@albinsuresh albinsuresh temporarily deployed to Test Pull Request March 26, 2025 17:45 — with GitHub Actions Inactive
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 69.31818% with 27 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ates/core/tedge_agent/src/entity_manager/server.rs 34.61% 17 Missing ⚠️
...rates/core/tedge_agent/src/entity_manager/tests.rs 85.24% 7 Missing and 2 partials ⚠️
crates/core/tedge_api/src/entity_store.rs 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@albinsuresh albinsuresh marked this pull request as ready for review March 26, 2025 18:12
@albinsuresh albinsuresh self-assigned this Mar 27, 2025
@albinsuresh albinsuresh temporarily deployed to Test Pull Request March 27, 2025 06:23 — with GitHub Actions Inactive
Comment on lines +202 to +206
if let Channel::EntityTwinData { fragment_key } = channel {
let fragment_value = serde_json::from_slice(message.payload_bytes())?;
let twin_message = EntityTwinMessage::new(topic_id, fragment_key, fragment_value);
self.entity_store.update_twin_fragment(twin_message)?;
}
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek Mar 27, 2025

Choose a reason for hiding this comment

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

I'm surprised by this bit. This is unrelated to the goal of this PR (clearing retained twin data messages on delete). And wasn't twin data already processed by the agent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was surprised by that too. This bit got missed during the entity store migration. Writing the test for the bug highlighted this gap. This was never spotted because the entity cache of the mapper was still processing it properly. The twin_data maintained by the agent was somewhat unused so far, until now.

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

Copy link
Copy Markdown
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Approved. Nice addition of the retained keyword for RobotFramework system tests.

@reubenmiller reubenmiller changed the title fix: Clear retained entity twin messages on deregister fix: clear retained entity twin messages on deregister Apr 3, 2025
@albinsuresh albinsuresh force-pushed the fix/entity-deregister-clear-all-data branch from cd477e6 to bc0d8be Compare April 11, 2025 09:07
@albinsuresh albinsuresh temporarily deployed to Test Pull Request April 11, 2025 09:07 — with GitHub Actions Inactive
@albinsuresh albinsuresh force-pushed the fix/entity-deregister-clear-all-data branch from bc0d8be to 46bec66 Compare April 12, 2025 08:11
@albinsuresh albinsuresh temporarily deployed to Test Pull Request April 12, 2025 08:11 — with GitHub Actions Inactive
@albinsuresh albinsuresh added this pull request to the merge queue Apr 12, 2025
Merged via the queue into thin-edge:main with commit 0c21166 Apr 12, 2025
34 checks passed
@albinsuresh albinsuresh deleted the fix/entity-deregister-clear-all-data branch April 24, 2025 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

theme:entity_store Entity store related functionality theme:mqtt Theme: mqtt and mosquitto related topics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants