fix IPersistencePlugin inconsistency #2576
Conversation
| } | ||
| } | ||
| } | ||
| if (commitExceptions != null) throw new AggregateException(commitExceptions); | ||
| snapshot.Commit(); |
There was a problem hiding this comment.
If snapshot is the last to commit then #2570 (comment) is probably not relevant though, plugins won't ever have N-1 data, rather they could have some N+1, but they won't process it again because of plugin.CurrentHeight. So it should be fine.
| { | ||
| plugin.OnCommit(system, block, snapshot); | ||
| plugin.OnPersist(system, block, snapshot, all_application_executed); |
There was a problem hiding this comment.
Maybe OnPersist and OnCommit could be merged then? Previously they were done at different stages of processing, but now they're very similar and probably this separation is not really needed. It will change plugin interface though (but I doubt we have a lot of third-party plugins now).
There was a problem hiding this comment.
This is introduced in #568:
If there are multiple persistence plugins loaded and one of them throws an exception, then none of them should actually commit anything to a persistent store. To facilitate this behavior, I added an OnCommit method to be called after all plugins have had a chance to process results from OnPersist and throw if something was wrong.
There was a problem hiding this comment.
I think we'd better keep this strategy to make sure there is no dirty data stored in plugins once some error occurs
There was a problem hiding this comment.
But it's better to revert if there is an error
There was a problem hiding this comment.
Once we merge these 2 functions, it will not be able to work correctly for such scenarios: i.e. there are 2 IPersistencePlugins installed. The first one persists without error but the second one throws some exception caused by dirty data. We can restart nodes again to re-persist data for neo-node and IPersistencePlugins, but the first plugin is already "polluted", I'm afraid the node can only be re-syncronized from 0 height again...
There was a problem hiding this comment.
If there are two plugins, and one throw an exception the problem will be in the plugin, I still thinking that is better to merge it. What do you think @erikzhang ?
There was a problem hiding this comment.
@erikzhang Could you provide opinion upon this question?
|
Ping? |
|
Can we close this? This is so outdated. |
This can be closed and open a issue related to https://github.com/neo-project/neo/pull/2576/files#r690242355 |
|
But dont we use |
Yes, seems that it was already merged |
Close #2570
Furthermore, we should regulate that IPersistencePlugin.OnCommit & IPersistencePlugin.OnPersist should never change the content of snapshot