Add new deleteObject functions to Persistence API

[pjfanning]

See apache/pekko-persistence-jdbc#156 for background.

The idea is that Future[Done] isn't informative enough as a return type.

Unfortunately, doing a Future.failed and throwing an exception if nothing is deleted is a behaviour change even if the return type allows this. Existing Akka/Pekko users could justifiably to be unhappy if we change the existing API to fail when there are no rows to delete instead of returning a Success with Done.

Ideally any new functions should default to calling the existing deleteObject API functions so that we don't force every Persistence implementation to uptake it.

[pjfanning]

@Roiocam @kerr @mdedetrich Let's delay the 1.1 releases while we work this out. I would prefer not to rush any changes to our APIs.

[He-Pin]

Maybe we can make of of something like Future[Seq[DeleteObjectResult]]`

[Roiocam]

TL;DR: NOT need to modify APIs (both user-facing or internal). If deletion or update results are ineffective,
we only need to throw an exception and allow Actors to recovery states from databases for further processing.

The user API was started from Effect, there is no any API about delete in EventSourced mode, it was DurableState specific.

I think we need to consider API and our promise about it.

In EventSourced:

• DAO(database access) shouldn't be unsuccessful, if it does, it will throw an exception and restart, rebuild state via event sourcing.
• The delete method is used internally, it only calls when a snapshot is created.

What about DurableState? for now:

• not fully supporting CRUD, only implementing C,R,D, and not allowing updates.
• delete should be always successful until the exception throw.

Based on the known assumptions mentioned above (which may not be entirely correct), I am curious about the guarantee of deletion operations in DurableState under network partition. Before that, let's discuss network partitioning under EventSourcing:

Actors will continuously increment versions, and ID + Version is a unique primary key. In case of network partition, the following scenario may occur:

• Network partition occurs, and both partitions hold the in-memory state [A,9].
• Partition 2 receives a request and writes an event with ID and version [A,10].
• Partition 1 also receives a request. Since the in-memory version of A is still 9, it will also attempt to write an event with ID and version [A,10].
• At this point, there will be a primary key conflict in the database plugin. Partition 1 fails to write and needs to recovery the state [A,10] from DB. After this processing ends, it will append a new event with version 11.

In EventSourced mode, data integrity is implicitly protected by the database. But what about DurableState?

• Network partition occurs, and both partitions hold the in-memory state [A，9].
• Partition 2 receives a request and updates the state to [A，10].
• Partition 1 also receives a request. Since A's in-memory version is still 9 at this time when receiving new requests with inconsistent states, the resulting effect would be incorrect as well.

Therefore any updates or deletions should be rejected so that Actors can recover from exceptions by reading from
the database for state recovery.

So I believe we might not need to modify APIs (both user-facing or internal). If deletion or update results are ineffective,
we only need to throw an exception and allow Actors to recovery states from databases for further processing.

[pjfanning]

To be clear, I don't support an API change either. I am getting a lot of suggestions to make API breaking changes in apache/pekko-persistence-jdbc#156. I would prefer not to change the result behaviour. I simply want to respect the revision param. My preference is to not worry if no rows are deleted, as is the case in the v1.0 API.

[Roiocam]

I think we don't need to change the API, just throw an exception and then let pekko core handle it(mostly it will restart and replay )

pekko/persistence-typed/src/main/scala/org/apache/pekko/persistence/typed/state/internal/DurableStateStoreInteractions.scala

Lines 79 to 81 in ebf5c89

	ctx.pipeToSelf[Done](setup.durableStateStore.deleteObject(persistenceId, newRunningState.revision)) {
	case Success(_) => InternalProtocol.DeleteSuccess
	case Failure(cause) => InternalProtocol.DeleteFailure(cause)

[mdedetrich]

Okay then this all seems fine with me, just to confirm

• For 1, we will add a new method into pekko core to handle these new cases. Also a new exception type should be added if an existing one for NotImplemented doesn't exist since this will be the default implementation for the method
• For 2, we will just throw an exception to handle these cases. This exception will also need to be added into pekko core since pekko core needs to handle it

[pjfanning]

All in all, we are very possibly heading towards calling the next Pekko release 2.0.0.
I don't like treating deleting no rows as a failure - especially since Pekko 1.0 doesn't treat it as failure.
I would still argue that Future[Boolean] is a better return type than Future[Done]. The boolean result is true if a deletion happened and false if no row was deleted. A DB Exception would lead to a Future failed state wrapping the exception.
I would prefer the new return type to be on new functions and to deprecate the existing deleteObject functions.

I would argue strongly against the name NotImplemented. I know of APIs that use the term NotImplemented as meaning Unsupported. NothingDeleted or something like that would make more sense to me. But I think Boolean is simpler still.

[mdedetrich]

I would argue strongly against the name NotImplemented. I know of APIs that use the term NotImplemented as meaning Unsupported. NothingDeleted or something like that would make more sense to me. But I think Boolean is simpler still.

NotImplemented is the most correct terminology here as its just about adding a default method in pekko-core so we don't break binary compatibility you need a default implementation so, i.e. in pekko core there will be new method

def deleteObject(...) = throw NotImplemented

This is not the same as NotSupported as NotSupported is something that an implementation (i.e. jdbc, r2dbc) should throw as its explicitly saying that "we can confirm that the database cannot support this" which is different to "this method is currently not implemented by this implementation". Same deal with NothingDeleted

NotImplemented is meant to be a stub whereby eventually all implementations of persistence will override with something more concrete and I cannot think of a more accurate name for this. At least to me other API's misusing terminology is not a convincing argument. We can also just use ??? and this seems to have been done in other places in pekko ecosystem

Either that or we just add the new method in Pekko 2 and then it doesn't need a new implementation as we can guarantee that all implementations of persistence will implement this method concretely

[pjfanning]

If that's what you want, why not https://docs.oracle.com/javase/8/docs/api/java/lang/UnsupportedOperationException.html ?

I would still argue that the new delete functions can call the existing (probably to be deprecated) delete functions - by default. We only have 4 implementations so it shouldn't take us long to override the default implementation with the correct implementations.

[pjfanning]

@jtjeferreira I'm wondering if you might have an opinion on this. I can see the point about exposing if the delete calls have no impact but at the same I would worry that changing the behaviour of the existing API to potentially fail if no rows are deleted may be unexpected by users used to the existing solution.

[raboof]

The 'revision' parameter was added in 66afe3f#diff-98235104917c7f9b25edc73a7d095c7b16a7047a9a6737d58198724b560f0af3 .

If I recall correctly, the point of the revision is to act as an optimistic lock: someone looking to update the record would read the current state and revision, make changes to the state, and write it with the 'next' revision number. If a different writer had written to the same state in the mean time, the write fails (and can be retried taking into account the 'newly-written' state if necessary).

I think it would be consistent with the API for persist to have the delete fail when the revision number does not match (meaning there was a concurrent write between reading the value and deleting it).

I'm not convinced existing Akka/Pekko users would be "justifiably unhappy" if we change the existing API behaviour in this way, I think we can see this as a bugfix: AFAIU older releases would 'incorrectly' delete data when the revision did not match, and they should've handled failures to delete data regardless. IMHO a warning in the release notes would be sufficient.

[raboof]

I think it would be consistent with the API for persist to have the delete fail when the revision number does not match (meaning there was a concurrent write between reading the value and deleting it).

to be clear: my proposal would be to return a failure only when an active record with a mismatching revision number is found, but keep returning Success(Done) if the entity was already deleted.

[pjfanning]

What would the exception be in the failure case? Will we need to create a new Exception class for this?

[raboof]

looks like upsertObject just fails with an IllegalStateException, so that might be fine - though a new Exception class might be slightly nicer?

[pjfanning]

feels like an OutOfDateRevisionException or something like might be more informative.

[pjfanning]

Design Proposal for Pekko 1.1 changes

• Add OutOfDateRevisionException and UnknownRevisionException to Pekko Persistence API
• We will fail with OutOfDateRevisionException if the revision in the deleteObject is out of date
• We will fail with UnknownRevisionException if the revision in the deleteObject call is ahead of the saved revision for the persistenceId
• Change all 4 Persistence implementations (Cassandra, JDBC, R2DBC, DynamoDB) to respect the revision in deleteObject calls
• We will use Java Reflection to create OutOfDateRevisionException and UnknownRevisionException to maintain Pekko 1.0 compatibility
• If the Persistence v1.1 implementations are used with Pekko 1.0, they will not fail if the revision is out of date or unknown
• Add migration/release notes for Pekko 1.1 specifying that deleteObject(persistenceId, revision) will respect the revision number and fail with OutOfDateRevisionException if the revision is not correct
• the deprecated deleteObject(persistenceId) will not be changed and will continue to not fail if nothing is deleted

wdyt @raboof @mdedetrich @Roiocam @kerr

[He-Pin]

1. So when UnknownRevisionException is thrown what should a user do? What if the data is persisted without a revision?
2. as user can use pekko 1.0 with a plugin built with pekko 1.1.0, will that cause a problem eg: override nothing issue?

[pjfanning]

1. Durable state always has revision.
2. the changes will be tested with pekko 1.0 and 1.1 to ensure that the code works with pekko 1.0 and 1.1

[pjfanning]

@Roiocam @He-Pin apache/pekko-persistence-jdbc#156 isn't exactly un take of these changes. In PR156, I just use DurableStateStoreException as this PR is not published yet. That PR makes me think I want to revisit the design a bit.

• The check in PR156 to decide on what type of exception to throw when the delete doesn't work - I think this has a race condition because the state could change because a different concurrent process updates the object after the delete attempt
• So I think we just need one exception a RevisionDeleteExecption which is thrown if the delete does nothing and no attempt is made to work out why the delete failed
• One other thing is that PR156 causes me to add a dependency in pekko-persistence-jdbc on pekko-persistence-typed - is this ok? There is an argument that the new exception could be put in the pekko-persistence jar and DurableStateStoreException could be made into a subclass of a shared parent exception.

[He-Pin]

As point 3,we should make the dependency just as the same, so add a new dependency is a no, sorry not noticed that.