-
Notifications
You must be signed in to change notification settings - Fork 499
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
ingest/ledgerbackend: Restart Stellar-Core when it's context is cancelled #4192
ingest/ledgerbackend: Restart Stellar-Core when it's context is cancelled #4192
Conversation
func (c *CaptiveStellarCore) isClosed() bool { | ||
return c.closed | ||
} |
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.
Could we simplify this to include the above condition, instead? In other words,
func (c *CaptiveStellarCore) isClosed() bool {
return c.closed || c.stellarCoreRunner == nil || c.stellarCoreRunner.context().Err() != nil
}
Basically, is there a situation in which the instance would not be closed and the runner wouldn't exist? Intuitively seems like no, but I'm not strong on this part of the codebase.
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.
Adding extra conditions to isClosed
won't work. In #4088 we wanted to separate two cases:
CaptiveCoreBackend
closed - means no usable, you need to create a new instance.stellarCoreRunner
closed - means it just needs to be restarted by callingPrepareRange
.
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.
Got it, thanks for clarifying! LGTM 👍
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
After refactoring in #4088 (and as a result of my wrong comment: #4088 (comment)) the
CaptiveCoreBackend.isPrepared
method returnedtrue
ifstellarCoreRunner
process was shutdown without callingclose()
- so in case of binary update but also in case of Stellar-Core crash.This commit fixes this bug by checking if
stellarCoreRunner
context was cancelled (meaning Stellar-Core is closed or closing but not as a result ofclose
call). I also removedisClose
method because it was simply checkingclose
variable. All test changes are only adding an extracontext()
call mocks becauseisPrepared
now calls it.