-
Notifications
You must be signed in to change notification settings - Fork 1k
StorageDumper: make dump format compatible with StatesDumper #3908
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
base: master
Are you sure you want to change the base?
Conversation
Problem: #3281 removes StatesDumper plugin, however, the output format of state dump files generated by StorageDumper is incompatible with StatesDumper format. We have a set of existing tools that rely on output format of StatesDumper. Solution: Get back to the old format of output dump files. Signed-off-by: Anna Shaleva <[email protected]>
c935ae8
to
0b436ae
Compare
We can adapt our tools, but check with NGD as well. |
} | ||
|
||
private void InitFileWriter(uint network, IReadOnlyStore snapshot) | ||
void OnCommitStorage(uint network, NeoSystem system) |
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 change IReadOnlyStore
to NeoSystem
?
There is only one system.GetSnapshotCache()
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.
The event receive NeoSystem
I strongly dislike this PR. I rarely say sth like this, but This PR is a real BUG. |
It's an API/format that is publicly available, so it's not that easy. Take RPC for example, it's a plugin, but this doesn't mean you can change its behavior easily, a lot of other code depends on it. This one of course is not that popular, but still. Now if there is a real bug with the format (but I'd check if it's a format problem rather than implementation) we can consider switching to |
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.
@AnnaShaleva ,create another plugin for now. We need to release soon, leave this plugin as it is. Later we decide how to merge.
At least we shouldn't bring bug back. Or I think you need is just making StatesDumper back. Then just revert it but don't change StorageDumper. |
Description
Problem:
#3281 removes StatesDumper plugin, however, the output format of state dump files generated by StorageDumper is incompatible with StatesDumper format. We have a set of existing tools that rely on output format of StatesDumper.
Note that it was said in #3271 (comment) that:
But it's not true, because all our tools use the output of StatesDumper actually.
Solution:
Keep the StatesDumper output format compatible with StorageDumper.
Type of change
How Has This Been Tested?
Checklist: