-
Notifications
You must be signed in to change notification settings - Fork 5
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
#197 Invoke actions via ActionInvoker #203
Conversation
Populate exception details in FragmentResult and in invocation logs.
Codecov Report
@@ Coverage Diff @@
## master #203 +/- ##
============================================
+ Coverage 92.07% 92.23% +0.15%
- Complexity 1905 2000 +95
============================================
Files 181 192 +11
Lines 7432 7572 +140
Branches 180 176 -4
============================================
+ Hits 6843 6984 +141
- Misses 502 513 +11
+ Partials 87 75 -12
Continue to review full report at Codecov.
|
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.
✔️ but consider my comment
@@ -70,6 +77,17 @@ public static FragmentResult fail(Fragment fragment, String errorCode, String er | |||
FragmentOperationFailure.newInstance(errorCode, errorMessage)); | |||
} | |||
|
|||
// Should be used only by caller to indicate synchronous or asynchronous exception |
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.
Since this is a comment explaining how the method should be used, maybe it should be a javadoc
FragmentOperationFailure.newInstance(error)); | ||
} | ||
|
||
// Should be used only by caller to indicate external timeout |
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.
Same here
@@ -152,6 +170,19 @@ public FragmentOperationFailure getError() { | |||
return error; | |||
} | |||
|
|||
public boolean isSuccess() { |
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.
Either:
isSuccess
andisError
andisException
orisSuccessful
andisErroneous
andisExceptional
HTTP Action docs update. Co-authored-by: Marcin <8897707+marcinus@users.noreply.github.com> Co-authored-by: Marcin <8897707+marcinus@users.noreply.github.com>
'}'; | ||
} | ||
|
||
public Status getStatus() { |
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 move those getters up.
return error; | ||
} | ||
|
||
public enum Status { |
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 move this inner class to the beginnning.
Populate exception details in FragmentResult and in invocation logs.
…otx-fragments into feature/enhance-invocation-logs
Description
Introduces
ActionInvocation
data object for storing more details about invoking action (duration, errors, cause of completion).Provides
ActionInvoker
interface as a single point of invoking actions, providing race prevention, time measurement and basic error handling (see JavaDoc for details).Introduces additional fields in
ActionInvocationLog
in order to pass on information about invoked actions.Motivation and Context
It is a milestone to implement #197. Also, it fixes some potential race issues not tracked directly in Knot.x GitHub.
Upgrade notes (if appropriate)
It is advised to switch from direct action calling to using
ActionInvoker
utility class.This ensures that the execution will not halt for some of the known issues in Vert.x and Knot.x implementations.
Types of changes
Checklist:
I hereby agree to the terms of the Knot.x Contributor License Agreement.