-
Notifications
You must be signed in to change notification settings - Fork 4
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
PLAT-671: send VDelegatedRequestFinished after retries for correct pending count #568
PLAT-671: send VDelegatedRequestFinished after retries for correct pending count #568
Conversation
…671_send_VDelegatedRequestFinished_after_retries
@@ -722,6 +722,8 @@ func (s *SMExecute) stepSendOutgoing(ctx smachine.ExecutionContext) smachine.Sta | |||
} | |||
} else { | |||
if s.outgoingSentCounter >= MaxOutgoingSendCount { | |||
// TODO when CallSummary will live longer than one pulse it needs to be updated | |||
s.sendDelegatedRequestFinished(ctx, nil) |
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.
I don't like the idea of setting "sendDelegatedRequestFinished" in every place where work can be stopped prematurely :)
I'd prefer to set "ErrorHandler"/"some kind of destructors" somewhere and catch all that situations at once.
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.
We discuss this with @ruz and decided that this is important for pending tables. Or I can't understand your idea in second sentence.
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.
I mean that part of code (send pending request finished) must be executed on every exit scenario after we've received VDelegatedCallResponse
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 that's a big addition, so I guess only if @ruz is OK
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.
Yeah, we need some kind of destructor for this.
…of github.com:insolar/assured-ledger into PLAT-671_send_VDelegatedRequestFinished_after_retries
Codecov Report
@@ Coverage Diff @@
## master #568 +/- ##
=========================================
Coverage ? 61.77%
=========================================
Files ? 691
Lines ? 47068
Branches ? 0
=========================================
Hits ? 29075
Misses ? 15833
Partials ? 2160
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.
Can you, also, open a ticket for some kind of destructor for SMExecute like using sm.stepSafeStop()
or sm.stepSafeError
? Looks like something important in the future.
add send, add log in handler for empty latestState case, actualize test