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

Message send state #669

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Message send state #669

merged 6 commits into from
Apr 18, 2024

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Apr 17, 2024

Part of #516

  • Add enum field of failed to delivery status for when unpublished never publishes
  • Expose delivery status
  • Expose delivery status filtering on find_messages
  • Expose message id on send

In a follow up PR we need to figure out how to set the delivery status to failed after 3 unsuccessful retries to publish. @tuddman maybe you have some ideas on how to do that?

@nplasterer nplasterer self-assigned this Apr 17, 2024
@nplasterer nplasterer requested a review from a team as a code owner April 17, 2024 20:25
@tuddman
Copy link
Contributor

tuddman commented Apr 17, 2024

maybe you have some ideas on how to do that?

Keeping a counter of attempts is pretty straightforward; in-memory or even stored-on-disk, Exponential back-off, too, if we want to get fancy.
I'm keen to learn in what condition(s) publishing could or would fail, and see if we can ensure that never happens.
(basically making it more correct over hardening it from when it's wrong/broken/failing. We can do both, but I'd want to start with the former)

@nplasterer
Copy link
Contributor Author

In the follow up PR we should do the following

  1. Mark the optimistic message as FAILED whenever its intent is marked as failed (set_group_intent_error)
  2. Make sure that every possible error case in this method results in the corresponding intent either being marked as TO_PUBLISH or FAILED
  3. At some point in the future revisit whether there is a possibility of an infinite loop of intent retries. Maybe we should call increment_intent_publish_attempt_count() wherever we reset an intent to TO_PUBLISH, and just set a really high retry limit, like 10?

For (2), maybe the most fool-proof way is to mark all MessageProcessingErrors as either retryable or non-retryable, and have the caller of the method catch all errors and mark the intent as TO_PUBLISH for retryable errors and FAILED for non-retryable errors

Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Looks great

@nplasterer nplasterer merged commit 5b62701 into main Apr 18, 2024
6 checks passed
@nplasterer nplasterer deleted the np/message-send-state branch April 18, 2024 03:59
@nplasterer nplasterer mentioned this pull request Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants