Skip to content
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

Accept any type as the key of the downloader actor #2291

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

rina23q
Copy link
Member

@rina23q rina23q commented Sep 26, 2023

Proposed changes

This change is required for c8y-mapper's enhancement to download a file per an operation. For example, if the key is only string, it's difficult to distinguish which operation (software, firmware, config_update) fires the download request.

So, this kind of example usage will be possible with this change, sending DownloadRequest:

struct DownloadKey {
    op_type: OperationType,
    id: String
}

self.message_box
    .download_sender
    .send((DownloadKey{ op_type: OperationType::ConfigUpdate, id: "abcd1234".into() }, download_request))
    .await?;

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

#2292

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 September 26, 2023 19:38 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #2291 (9bdfdc7) into main (300c43d) will decrease coverage by 0.5%.
The diff coverage is 90.6%.

❗ Current head 9bdfdc7 differs from pull request most recent head dc2bf1a. Consider uploading reports for the commit dc2bf1a to get more accurate results

Additional details and impacted files
Files Coverage Δ
...rates/extensions/tedge_downloader_ext/src/actor.rs 79.6% <50.0%> (-1.3%) ⬇️
...rates/extensions/tedge_downloader_ext/src/tests.rs 93.4% <92.6%> (-0.4%) ⬇️

... and 16 files with indirect coverage changes

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
294 0 3 294 100 1h1m53.959s

This change is required for c8y-mapper's enhancement to download a file
per an operation. For example, if the key is only string, it's difficult
to distinguish which operation (software, firmware, config_update) fires
the download request.

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
@rina23q rina23q force-pushed the improve/downloader-takes-any-key branch from 9bdfdc7 to dc2bf1a Compare September 27, 2023 13:13
Copy link
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

@rina23q rina23q temporarily deployed to Test Pull Request September 27, 2023 13:25 — with GitHub Actions Inactive
@rina23q rina23q merged commit ffa9983 into thin-edge:main Sep 27, 2023
16 checks passed
@rina23q rina23q deleted the improve/downloader-takes-any-key branch September 27, 2023 13:59
@albinsuresh
Copy link
Contributor

Although I've got nothing against making the Downloader accept generic types as request keys, I fail to get the benefits from the mapper side.

The mapper already had to send a unique operation_id to trigger a download from this actor and hence the mapper already would have known what kind of operation_type it generated that unique id for. The mapper was already caching that ID in the reqs_pending_download map and it could have just added the OperationType info as well into this map. Instead, here we are sending that info to the downloader, which does nothing with that additional info, essentially using the downloader as that cache, whereas we could have simply extended the reqs_pending_download map for the same. Or did I miss something else here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants