Skip to content
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

Pass ApplicationExecutionResult to IPersistencePlugin #480

Closed
RavenXce opened this issue Nov 28, 2018 · 4 comments · Fixed by #531
Closed

Pass ApplicationExecutionResult to IPersistencePlugin #480

RavenXce opened this issue Nov 28, 2018 · 4 comments · Fixed by #531
Labels
enhancement Type - Changes that may affect performance, usability or add new features to existing modules.

Comments

@RavenXce
Copy link
Contributor

RavenXce commented Nov 28, 2018

Can we add pass execution_results to the IPersistencePlugins as well here?

List<ApplicationExecutionResult> execution_results = new List<ApplicationExecutionResult>();

plugin.OnPersist(snapshot);

We want to persist the notifications that happened, and don't want to use the async version in IActorRefs as we need guaranteed writes (we want to abort persist if our plugin fails).

Alternatively, having a blocking version of the Actor plugins would work too.

@erikzhang erikzhang added the enhancement Type - Changes that may affect performance, usability or add new features to existing modules. label Nov 28, 2018
@vncoelho
Copy link
Member

vncoelho commented Nov 28, 2018

@RavenXce, this is an useful thing, it would be nice to have this notification tool inside neo-cli as @localhuman suggested #352.

Now, considering this OnPersist plugin it will probably be easier and simpler.

EDIT:
@RavenXce, however, the abort part does not look so easy to revert and implement.
Is there really plenty of cases in which the plugin will fail?
Maybe you can handle this inside the plugin, you cache the notifications (namely execution_results) and perform your strategy.

@RavenXce
Copy link
Contributor Author

@vncoelho since OnPersist is blocking and called just before Commit, simply throwing when an unrecoverable error occurs (such as persistent failure to connect to a remote db) within the plugin should crash the whole app, aborting persisting of the block. An external supervisor can reboot the node if required.

@vncoelho
Copy link
Member

vncoelho commented Nov 28, 2018

I got, it, the external supervisor reboot is the part I thought to be more complex, anyway...

But why a notification plugin should avoid a block to persist if all verification's passed until that moment, would not it be a selective Block filter plugin? aehuaheuaea

@erikzhang
Copy link
Member

Because it may be that something went wrong, and we don't want to miss this block and resynchronize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type - Changes that may affect performance, usability or add new features to existing modules.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants