-
Notifications
You must be signed in to change notification settings - Fork 40
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
[R4R] Recover from last running mode when restarts #466
Conversation
0c57a63
to
a5f79a6
Compare
a5f79a6
to
feb42d5
Compare
related with bnb-chain/bnc-tendermint#34. Please notice merged order |
if mode == "0" { | ||
runtime.RunningMode = runtime.NormalMode | ||
runningMode = runtime.NormalMode |
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.
What concern to verify a query request?
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.
what did you mean here?
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.
To my understand, although this is a query request, it can set the node working mode. So the query request should provide the signature on once data to acquire the setting permission.
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.
even it's a query request, we need to check the permission
appConfig "github.com/binance-chain/node/app/config" | ||
"github.com/binance-chain/node/common/log" | ||
) | ||
|
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 make this file less dependency. Like func mustSaveToFile(cfg *config.Config, params *runtimeParams)
. to func mustSaveToFile(filePath string, params *runtimeParams)
.
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.
fixed.
feb42d5
to
022def7
Compare
if mode == "0" { | ||
runtime.RunningMode = runtime.NormalMode | ||
runningMode = runtime.NormalMode |
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.
what did you mean here?
params = mustReadFromFile(path) | ||
params.Mode = mode | ||
} | ||
mustSaveToFile(path, params) |
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.
why do we use mustSaveToFile here? does it mean if we failed to save, the process would crash?
I would rather return the error back to command line, so that the command fails, and the caller of the command should handle.
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.
if it failed to save to the file, it must be an internal error, we can just panic here and the recover()
logic will return an internal error to the client.
Description
Persistent the running mode to file when update. So we can recover from it when restarts.
Rationale
After setting to recovery mode, we would do some fix and restart the node, the node needs to run still in recovery mode when started.
Before this PR, we use a param in
StartCmd
to manually specify the mode. It's not friendly to our deployment scripts.Example
Changes
Notable changes:
*
Preflight checks
make build
)make test
)make integration_test
)Already reviewed by
...
Related issues
... reference related issue #'s here ...