Skip to content

Remove StatesDumper - Minor Refactor on StorageDumper#3281

Merged
shargon merged 5 commits intomasterfrom
organize-plugins-statesStorageDumper
May 28, 2024
Merged

Remove StatesDumper - Minor Refactor on StorageDumper#3281
shargon merged 5 commits intomasterfrom
organize-plugins-statesStorageDumper

Conversation

@vncoelho
Copy link
Member

close #3271

@vncoelho vncoelho requested review from cschuchardt88, roman-khimov and superboyiii and removed request for cschuchardt88 May 27, 2024 11:20
@vncoelho
Copy link
Member Author

@superboyiii @roman-khimov, I suspect that you are the ones that use this plugin.
I have been using StateDumper, but as I verified, StorageDumper was updated by @Ashuaidehao and it looks like you both are using StorageDumper.

In this case, I deleted StateDumper.

@Jim8y
Copy link
Contributor

Jim8y commented May 27, 2024

@superboyiii @roman-khimov, I suspect that you are the ones that use this plugin. I have been using StateDumper, but as I verified, StorageDumper was updated by @Ashuaidehao and it looks like you both are using StorageDumper.

In this case, I deleted StateDumper.

My suggestion is DO NOT DELETE IT DIRECTLT, but mark it as [Obsolete], no matter there is someone using it or not, we should not just delete an existing thing without telling the eco-system and warn them.

@vncoelho
Copy link
Member Author

vncoelho commented May 27, 2024

@superboyiii @roman-khimov, I suspect that you are the ones that use this plugin. I have been using StateDumper, but as I verified, StorageDumper was updated by @Ashuaidehao and it looks like you both are using StorageDumper.
In this case, I deleted StateDumper.

My suggestion is DO NOT DELETE IT DIRECTLT, but mark it as [Obsolete], no matter there is someone using it or not, we should not just delete an existing thing without telling the eco-system and warn them.

Both plugins are the same @Jim8y .
There is no sense the leave DUPLICATED code here in the core.

@vncoelho
Copy link
Member Author

I updated my last comment because I thought the message was from @superboyiii...aehuahea

There is no sense in what you said @Jim8y, you should check the code first.
As I said above.

@Jim8y
Copy link
Contributor

Jim8y commented May 27, 2024

I updated my last comment because I thought the message was from @superboyiii...aehuahea

There is no sense in what you said @Jim8y, you should check the code first. As I said above.

I am not saying you should keep duplicate code, what I mean is you should not delete existing thing directly. Its duplicated, but its also a neo product, a product should not be removed all of a sudden, but give a warning first, then remove later.

@vncoelho
Copy link
Member Author

I updated my last comment because I thought the message was from @superboyiii...aehuahea
There is no sense in what you said @Jim8y, you should check the code first. As I said above.

I am not saying you should keep duplicate code, what I mean is you should not delete existing thing directly. Its duplicated, but its also a neo product, a product should not be removed all of a sudden, but give a warning first, then remove later.

I understand your point of view. However, this is not the case now @Jim8y.

Furthermore, it is COMPLETELLY SIMPLE TO MODIFY to the other plugin instead, if someone is currently using the StateDumper.
However, I doubt someone is using it because perhaps StorageDumper fixed a minor problem that StateDumper had in some edge cases.

@Jim8y
Copy link
Contributor

Jim8y commented May 27, 2024

You can still do the same, i just suggest to do it latter, after one release. But i am ok if other coredevs think its fine.

@Jim8y Jim8y requested a review from a team May 27, 2024 23:40
@Jim8y
Copy link
Contributor

Jim8y commented May 27, 2024

I would suggest you write a readme to introduce the plugin.

@vncoelho
Copy link
Member Author

I would suggest you write a readme to introduce the plugin.

That a good idea. I will check how current Readme is.

@roman-khimov
Copy link
Contributor

If they have the same functionally and one can be replaced with another then deletion is fine and it'd just be a release note --- "if you're using X, please use Y now". And it's not a very popular plugin, not RpcServer for sure.

We have a SaveStorageBatch setting in NeoGo (https://github.com/nspcc-dev/neo-go/blob/master/docs/node-configuration.md) that exports the same data and then we have https://github.com/nspcc-dev/neo-go/blob/master/scripts/compare-dumps/compare-dumps.go to compare dumps which we do from time to time (I hope we'll be doing it less often after #1526). From what I see here, the dump format stays the same, so it's perfectly fine for us.

@shargon shargon merged commit df06791 into master May 28, 2024
@shargon shargon deleted the organize-plugins-statesStorageDumper branch May 28, 2024 08:06
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Apr 8, 2025
Block dump files were renamed from dump-block-X.json to
dump-block-X.dump in neo-project/neo#3281.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva added a commit that referenced this pull request Apr 20, 2025
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 <shaleva.ann@nspcc.ru>
AnnaShaleva added a commit that referenced this pull request Apr 20, 2025
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 <shaleva.ann@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove ONE OF THE duplicated PLUGINS StatesDumper and StorageDumper

4 participants