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.
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
Support retriable Chelonia actions #1768
Support retriable Chelonia actions #1768
Changes from 2 commits
c45ba09
421c340
21131ee
637e5a6
d357522
6192b3a
4ca17b0
9d14577
4222f8f
3b57a3c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Using
Date
is a bad idea for anything besides maybe logging. The reason is that the date could change arbitrarily. The better alternative is to use a monotonic timer for this, such asperformance.now()
. I'm not sure exactly what thisnextRetry
field is used for, exactly, though.The second issue is the, like for the
this.timer
assignment below, that1e3
seems like a pretty low value for a retryThere 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.
The
nextRetry
field is actually used for logging as per the issue requirement for'chelonia.persistentActions/status'
options.retrySeconds
is a number of seconds, but we need milliseconds here, hence the1e3
multiplierThere 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.
1e3 seems like a pretty low value
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.
Ditto
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.
Can we refactor this to use events instead of
options.successInvocationSelector
?I think it makes sense to add at least two events using
okTurtles.events
:CHEL_PERSISTENT_ACTIONS_SUCCESS
- would be emitted above hereCHEL_PERSISTENT_ACTIONS_FAILURE
- would be emitted next to line 82 above, next to theerrorInvocation
, along with theerror
objectAnd if you think of any others that we should emit, feel free to suggest
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.
Similarly, as this is a utility for Chelonia (and possibly in the future, an independent library), we cannot access
'state/vuex/getters'
eitherThere 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.
Ditto