-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
core/rawdb, eth, les,node: implemented update for unclean shutdown marker #22974
core/rawdb, eth, les,node: implemented update for unclean shutdown marker #22974
Conversation
…r every 5 minutes. Fixes #22580
Seems like the CI build fail is unrelated to this PR, I can't reproduce the fail locally. |
The PR is a good idea, but the problematic part is what you also mentioned in the code, that you have this dangling global channel to terminate the goroutine. We've discussed it a bit today that this mechanism might be better in the database layer, since "unclean shutdown" really means "was the db closed properly or not". We think the best place we could add it would be |
Thanks for the feedback! I will make the suggested changes. |
I have implemented what you suggested. |
I pushed a commit to clean it up a bit -- using a switch instead of a sequence of if-clauses, and removing an unnecesary param. It's a little smaller now, easier to evaluate. |
@BitBaseBit I am having this issue. How can I resolve it? |
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.
Huh, I made this comment a long++ time ago, but it got left in "Pending" -- I never actually submittted the review. Doh!
type nofreezedb struct { | ||
ethdb.KeyValueStore | ||
stopUncleanMarkerUpdateCh chan bool | ||
isChainDb bool |
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.
This is very un-intuitive IMO. Would be better to have the channel be nil
if it's not needed, instead of having a separate boolean on-the-side to tell whether it's used or not.
This PR was superseded by #24077. Closing. |
Fixes #22580. As mentioned there, the unclean shutdown marker was using the initial boot time of the last know crash to report to the user that a crash had occurred. It would be more useful if the user know when the crash happened, instead of only knowing when the node was booted before it crashed. As discussed in #22580, the marker update time is set to 5 minutes. There is also a global in there that I know shouldn't be, my initial though was to put it in handler in the Ethereum struct, but I'm not sure that's appropriate.