Allow persistence plugins to commit as a final step.

[jsolman]

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.

A persistence plugin implementing IPersistencePlugin should do any processing needed to stage data to persist in its OnPersist method, and then perform actual persistence in its OnCommit method. In this way database integrity is maximized.

[erikzhang]

However, if a plugin throws an exception on OnCommit(), the remaining plugins will not be able to execute the OnCommit() method properly.

[shargon]

Why do you need both cases? maybe we can remove OnPersist?

[erikzhang]

OnPersist() allows plugins to terminate the persistence process by throwing exceptions. I think that OnCommit() is not necessary.

[jsolman]

OnPersist() allows plugins to terminate the persistence process by throwing exceptions. I think that OnCommit() is not necessary.

There can be multiple persistence plugins. If there are 3 and last one that runs throws, and the other 2 have already persisted, it is a problem. If they do their processing in OnPersist and then do the work to actually write to disk in OnCommit the risk is minimized of getting in an inconsistent state due to some processing problem in OnPersist that could cause a plugin to throw an exception.

I changed the code so that OnCommit for the plugins happens before snapshot.commit is called since you expressed concern about them throwing in OnCommit.

[vncoelho]

I agree with @erikzhang . I think that this OnCommit() is not needed now.

@jsolman, while we have more plugins there are also additional switch cases on the OnPersist functions, thus, in such scenarios, they could communicate in that level as well, right?

[jsolman]

@vncoelho Why do you think it isn't needed right now? Some people have more than one persistence plugin? Why should an exception in processing notifications in the second plugin still allow the first to commit? This is what would happen now without having OnCommit() as a separate step?

[jsolman]

We can wait to merge this, but I'm still not seeing why it isn't helpful.

[vncoelho]

I think that you can add a switch case in the plugin itself.
Jeff, I do not see why to not parallelize the plugin in its file, if you need to run more than one.

[jsolman]

@jsolman, while we have more plugins there are also additional switch cases on the OnPersist functions, thus, in such scenarios, they could communicate in that level as well, right?

I’m not following you, even if there were a way for plugins to communicate from one OnPersist function to the next; the previous one could have already committed, the next that is going to throw an exception cannot undo it...

[jsolman]

@vncoelho Plugins are meant to be self contained. They should not have to coordinate on state and have some final plugin commit or something like that. That is a suboptimal design.

[erikzhang]

We can call OnCommit() after snapshot.Commit() and then catch the exceptions thrown by each plugin in OnCommit() so that at least the plugins don't interact with each other.

[jsolman]

@erikzhang Switching back to calling after snapshot.Commit() and catching exceptions seems reasonable to me, but maybe we should delegate whether it should crash the node to the plugin, because if we catch the exceptions the node operator might not realize their plugin is misbehaving occasionally.

Also, some node operators might really want a plugin commit failing to crash the node.

What about having a method in the plugin that gets called with the exception if it occurs during commit:

bool ShouldThrowExceptionFromCommit(Exception ex);

That method could return a value to specify whether or not the exception should be rethrown and crash the node. That way the plugin could choose to log the exception and not crash, or it could choose to have the exception rethrown.

Also it can wait till running all of the plugins OnCommit Methods, before actually rethrowing if one of the plugins chooses to throw and crash.

[vncoelho]

@jsolman, looks good now after the snapshot.commit (just now I got that this was your initial intention. When I revised it was aligned with the Persist.)