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
core/rawdb: refactor db inspector for extending multiple ancient store #25896
core/rawdb: refactor db inspector for extending multiple ancient store #25896
Changes from 1 commit
7ddb9f6
a9ae72a
23af804
1053fad
53340c2
8aa7349
863f611
6e6d50e
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.
Moving parts of the formatting code into the
freezerInfo
feels wrong. It is very very very specific on how this table is formatted here. I think it's a very low level impl detail leak into an API method (even if private). Please inline the formatting and use use getters onancient
to get the infos needed for the table.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
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 an interesting change, and not immediately obvious why it was needed (or correct). It would make a difference in case we're syncing, and in the old code we would create a goroutine to
waitBuild
, and in the new code we would not.Any comments about this?
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.
Previously, we will always create a waiter if "sync-style" snapshot construction is required. Although in production, sync-style generation is always disabled which means the waiter is never be created.
While if we are in sync, the snapshot construction is disabled, if we still wait the construction by waiter, then it's a deadlook.
This change ensures we only create the waiter that the construction is really initialized.
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 we merge this PR, the production code will start calling
obj.DeleteTrie
. It will soup up all the destructed storage slots intoset
, which get merged intonodes
,I think it will eventually get ignored in
trie.Database.Update
. Is that correct?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.
Yes. Perhaps we can run a full sync before merging?
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.
Good idea. I'll start one
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 now running on
03
against master on04
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.
Should be ok after running 6 days
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.
actually.. was just looking into this, and I don't think I ran the correct code
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.
restarted. sorry :(
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.
no worry, let's wait a few more days then