Message Failed State #902
Merged
Message Failed State #902
+92
−53
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neekolas
reviewed
Jul 15, 2024
| /// # Returns | ||
| /// Returns [`Option::None`] if [`StoredGroupIntent`] is not [`IntentKind::SendMessage`] or if | ||
| /// an error occurs during decoding of intent data for [`IntentKind::SendMessage`]. | ||
| pub fn message_id(&self) -> Result<Option<Vec<u8>>, IntentError> { |
There was a problem hiding this comment.
My only worry about this refactor is that people are going to use it and decode the proto somewhere else separately to get at the other fields, so we're going to end up doing the same work twice.
message_id sounds like it would be a cheap little operation, but it's actually pretty expensive.
There was a problem hiding this comment.
Added a comment explaining how that is a bad idea, we should watch out for that but I also am not sure when we would access the contents of a PlainText envelope outside of publish success/failure. It could be worth storing the message_id separately on StoredGroupIntent as an Option which would avoid the problem altogether
neekolas
approved these changes
Jul 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #671
added
message_idfunction toStoredGroupIntentthat calculates amessage_idif the intent is of typeSendMessage. On intent fail we can access the message and set it to failed inpublish_intents.Verified that the intent is only returned as failed from
publish_intentsonce retries have been reached. Any other error will leave intent as to_publish.Intent retries are limited by 3 in
sync_until_intent_resolvedand also to 3 inpublish_intents.