Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Mar 3, 2025

  • Use shorter variant of the same code
  • Clean up retry-tasks on shutdown

@Marenz Marenz requested a review from a team as a code owner March 3, 2025 16:51
@github-actions github-actions bot added the part:dispatcher Affects the high-level dispatcher interface label Mar 3, 2025
@Marenz Marenz requested a review from llucax March 4, 2025 08:49
@Marenz Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Mar 4, 2025
@Marenz Marenz enabled auto-merge March 4, 2025 08:50
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

A couple more comments and a couple related thoughts:

  1. Nitpick, but the class should also probably be called FailedDispatchesRetrier, now it is named as a function.

  2. Is there any reason to make retrying optional? Is there any case you can think of where retrying is not desired? It makes the code more complicated and I don't think we really need it.

  3. In the future I would like to move the retry module to the core lib so we can use it in more places, like here. If we do so, you can have a retry strategy that it is basically no retry, but the code will still be simple as the retry logic will not change.

  4. Lately I was also thinking about this frequenz-floss/frequenz-sdk-python#1176, but it is harder to think about something generally useful for an arbitrary background service.

@Marenz Marenz requested a review from llucax March 4, 2025 12:05
Marenz added 2 commits March 4, 2025 13:06
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz added this pull request to the merge queue Mar 4, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit ecaca06 Mar 4, 2025
14 checks passed
@Marenz Marenz deleted the bad-memories branch March 4, 2025 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR part:dispatcher Affects the high-level dispatcher interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants