-
Notifications
You must be signed in to change notification settings - Fork 12
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
[1.x] Allow to handle any RuntimeException
s occurring when delivering an InboxMessage
#1496
Merged
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
… any performance issues.
…llow the `DeliveryMonitor` instances access to more details.
… it could be used to diagnose any reception failures in scope of `Delivery` (in progress).
…ses in which there were no targets to route.
…ough endpoints into erroneous `DispatchOutcome`s.
armiol
requested review from
alexander-yevsyukov,
dmdashenkov and
yevhenii-nadtochii
January 2, 2023 16:23
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments and questions.
server/src/main/java/io/spine/server/aggregate/AggregateRepository.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/spine/server/delivery/AbstractMessageEndpoint.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/spine/server/delivery/FailedReception.java
Outdated
Show resolved
Hide resolved
server/src/test/java/io/spine/server/delivery/given/NoOpEndpoint.java
Outdated
Show resolved
Hide resolved
server/src/test/java/io/spine/server/delivery/given/ReceptionistAggregate.java
Outdated
Show resolved
Hide resolved
server/src/test/proto/spine/test/delivery/receptionist_events.proto
Outdated
Show resolved
Hide resolved
@alexander-yevsyukov PTAL again. |
alexander-yevsyukov
approved these changes
Jan 3, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
This changeset addresses #1472 and brings other improvements into the codebase.
Failure of signal reception, and
DeliveryMonitor
to respondInboxMessage
s which are processed in scope of eachDelivery
run, are eventually unpacked, and the respective signals are dispatched to their targets. In case a signal target is anEntity
, it is possible that the signal reception does not go well for various reasons. In particular, an aggregate may have "broken"@Apply
-er, making it impossible to load the aggregate state. Or, some other unexpected error may occur upon loading or storing an entity.Previously, it was impossible to trace such failures of signal reception. And the
InboxMessage
s causing the errors were left in their inboxes inTO_DELIVER
status forever, being delivered over and over again — most likely with the same reception failure repeating.Now, the
Delivery
API allows to subscribe for any failures which occur during the reception of each signal. Additionally, end-users may now choose the way to handle the reception failures in terms of action in respect to theInboxMessage
of interest.Out-of-the-box, there are two actions provided:
InboxMessage
as delivered — so that it does not block further delivery of messages;InboxMessage
. Please note, this is a synchronous and immediate action in terms of a delivery run.Alternatively, end-users may implement their own way of handling the reception failure.
The corresponding functionality is provided via the API of
DeliveryMonitor
.Here is a code sample:
By default,
InboxMessage
s are marked asDELIVERED
in case of failure of their reception. This is so, because otherwise the issue described in #1472 arises.Internally, such a functionality became possible by making
MessageDispatcher.dispatch(envelope)
to returnDispatchOutcome
. Previously, it wasvoid
, which was not suitable for handling any of the issues synchronously — as it was required forInboxMessage
to allow users choose the way to handle the reception failures.Other changes
It was reported, that at run-time the routing of events by their interfaces may lead to
ConcurrentModificationException
.This PR addresses the issue for
v1
by making theMap
of signal routes concurrency-friendly. Seeio.spine.server.route.MessageRouting
for more details.Even more minor changes are made to fix typos and remove the unused code.
The library version is set to
1.9.0-SNAPSHOT.6
.