-
Notifications
You must be signed in to change notification settings - Fork 143
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
feat: Notify application when a self-ISS event is detected by the platform #17274
Conversation
…platform Signed-off-by: Tim Farber-Newman <tim.farber-newman@swirldslabs.com>
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
...s-platform-core/src/main/java/com/swirlds/platform/state/iss/internal/DefaultIssHandler.java
Outdated
Show resolved
Hide resolved
...ore/swirlds-platform-test/src/test/java/com/swirlds/platform/test/state/IssHandlerTests.java
Outdated
Show resolved
Hide resolved
...s-platform-core/src/main/java/com/swirlds/platform/state/iss/internal/DefaultIssHandler.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17274 +/- ##
============================================
+ Coverage 67.23% 67.34% +0.10%
- Complexity 21988 22021 +33
============================================
Files 2583 2584 +1
Lines 96320 96310 -10
Branches 10058 10054 -4
============================================
+ Hits 64764 64860 +96
+ Misses 27847 27741 -106
Partials 3709 3709
|
...ore/swirlds-platform-test/src/test/java/com/swirlds/platform/test/state/IssHandlerTests.java
Outdated
Show resolved
Hide resolved
...orm-core/src/main/java/com/swirlds/platform/system/state/notifications/AsyncIssListener.java
Outdated
Show resolved
Hide resolved
…fication to it in DefaultAppNotifier Signed-off-by: Tim Farber-Newman <tim.farber-newman@swirldslabs.com>
...-node/hedera-app/src/main/java/com/hedera/node/app/state/listeners/FatalIssListenerImpl.java
Outdated
Show resolved
Hide resolved
...orm-core/src/main/java/com/swirlds/platform/system/state/notifications/FatalIssListener.java
Outdated
Show resolved
Hide resolved
...orm-core/src/main/java/com/swirlds/platform/system/state/notifications/FatalIssListener.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Tim Farber-Newman <tim.farber-newman@swirldslabs.com>
d26cffa
Signed-off-by: Tim Farber-Newman <tim.farber-newman@swirldslabs.com>
Signed-off-by: Tim Farber-Newman <tim.farber-newman@swirldslabs.com>
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. Thanks!
Signed-off-by: Tim Farber-Newman <tim.farber-newman@swirldslabs.com>
ef275a5
...-node/hedera-app/src/main/java/com/hedera/node/app/state/listeners/FatalIssListenerImpl.java
Show resolved
Hide resolved
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 from services changes. Just had one comment. Thanks @timfn-hg!
Also, to test this you could simulate submitting self-ISS event locally in platform code and run node using ./gradlew run
, submit some transactions. You should be able to see the warn log in hgcaa.log
.
Description:
When a self or catastrophic ISS event is detected by the platform, forward the event to the application for additional handling. This PR creates a new ISS listener called
AsyncFatalIssListener
that compliments the existingIssListener
. The new listener was created such that the event could be forwarded asynchronously, as opposed to synchronously with the originalIssListener
. If this async nature is not really wanted, then we can fall back to using the originalIssListener
(I think).Currently, the handler that receives the notification doesn't do anything meaningful, but follow up work will expand its use.
Related issue(s):
Fixes #17266
Notes for reviewer:
I've updated the platform tests to ensure the notification is forwarded in the case of ISS events, but I am not familiar with how to test the full path between the platform and app layer to ensure the notification is being received (i.e. making sure the app pieces are set up correctly). If anyone can point me to how to do this, that would be great.
Checklist