You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "pjfanning (via GitHub)" <gi...@apache.org> on 2024/03/24 11:43:36 UTC
[D] Add new deleteObject functions to Persistence API [pekko]
GitHub user pjfanning created a discussion: Add new deleteObject functions to Persistence API
See https://github.com/apache/pekko-persistence-jdbc/pull/156 for background.
The idea is that `Future[Done]` isn't informative enough as a return type.
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.
GitHub link: https://github.com/apache/pekko/discussions/1233
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
GitHub user mdedetrich edited a comment on the discussion: Add new deleteObject functions to Persistence API
> 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
```scala
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](https://github.com/apache/pekko-connectors/blob/fc4417af82c7c22439cf6f7e5a1e87298d0e2107/google-common/src/main/scala/org/apache/pekko/stream/connectors/google/scaladsl/%60X-Upload-Content-Type%60.scala#L48)
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
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913583
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
What would the exception be in the failure case? Will we need to create a new Exception class for this?
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913939
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning edited a discussion: Add new deleteObject functions to Persistence API
See https://github.com/apache/pekko-persistence-jdbc/pull/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.
GitHub link: https://github.com/apache/pekko/discussions/1233
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
GitHub user raboof edited a comment on the discussion: Add new deleteObject functions to Persistence API
The 'revision' parameter was added in https://github.com/apache/pekko/commit/66afe3fefb99dbc3e9e34755f28f4811b7624e86#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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913778
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
GitHub user mdedetrich edited a comment on the discussion: Add new deleteObject functions to Persistence API
> But as far as PR#[apache/pekko-persistence-jdbc#156](https://github.com/apache/pekko-persistence-jdbc/pull/156) is concerned, it also has a different behavior of delete in 1.0. Once the PR is merged, it will cause the possibility that the user expects to delete but does not delete.
Just wanted to point out that changing the behaviour is not out of the question as long as its warranted and ideally the breaking behaviour is more strict then the current behaviour so its easy to detect, i.e. Pekko 1.1.x [has such a breaking change ](https://pekko.apache.org/docs/pekko/snapshot/migration/migration-guide-1.0.x-1.1.x.html#splitwhen-splitafter-behavior-change)
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8907818
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
GitHub user Roiocam added a comment to the discussion: Add new deleteObject functions to Persistence API
But as far as PR#https://github.com/apache/pekko-persistence-jdbc/pull/156 is concerned, it also has a different behavior of delete in 1.0. Once the PR is merged, it will cause the possibility that the user expects to delete but does not delete.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8899545
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
feels like an OutOfDateRevisionException or something like might be more informative.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8914027
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
GitHub user Roiocam added a comment to the discussion: Add new deleteObject functions to Persistence API
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 )
https://github.com/apache/pekko/blob/ebf5c89a83d75642383c0d255c108204db1eea3d/persistence-typed/src/main/scala/org/apache/pekko/persistence/typed/state/internal/DurableStateStoreInteractions.scala#L79-L81
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8912163
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
GitHub user He-Pin added a comment to the discussion: Add new deleteObject functions to Persistence API
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?
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-9019058
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
GitHub user Roiocam added a comment to the discussion: Add new deleteObject functions to Persistence API
I mean, instead of quietly changing the deletion behavior (that is, deletion may not be successful), it is better to explicitly let users know (such as exceptions) that we can not change the API.
deleteObject is not directly for users, but Effect. Assuming that we need to implement a mechanism to notify users, it will make major changes to the API of the Pekko Persistence module.
I would like to divide this discussion into two parts:
1. For plugin developers is **Adding the deleteObject API**, I have no objections to it. It may require handling the changes in behavior for API, but we can also add default behavior in Pekko (such as exceptions) to reduce migrate work that something will block our releases. Over time, we can write appropriate behaviors for each plugin.
2. For users, the change in apache/pekko-persistence-jdbc#156 will cause a problem with ineffective deletion, and currently pekko persistence does not have any such API to notify users (I don't think it's a good idea to let users handle it; akka believes in "let it crash").
wdyt @pjfanning @mdedetrich
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8909747
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
GitHub user raboof edited a comment on the discussion: Add new deleteObject functions to Persistence API
> 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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913848
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
@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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8892286
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
GitHub user He-Pin added a comment to the discussion: Add new deleteObject functions to Persistence API
Maybe we can make of of something like Future[Seq[DeleteObjectResult]]`
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8892384
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
## Design Proposal for Pekko 1.1 changes
* Add OutOfDateRevisionException and UnknownRevisionException
* 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
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8975620
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
GitHub user mdedetrich added a comment to the discussion: Add new deleteObject functions to Persistence API
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
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8912779
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913481
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning edited a comment on the discussion: Add new deleteObject functions to Persistence API
For me, if we start throwing exceptions when no rows are deleted - that is a big behaviour change. Existing v1.0 code ignores it. No user would expect us to change this in a minor release. New function needed if we seriously want to change this.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8894056
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning edited a discussion: Add new deleteObject functions to Persistence API
See https://github.com/apache/pekko-persistence-jdbc/pull/156 for background.
The idea is that `Future[Done]` isn't informative enough as a return type.
Unfortunately, doing a Future.failed and throw 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.
GitHub link: https://github.com/apache/pekko/discussions/1233
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
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
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-9019138
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
GitHub user raboof added a comment to the discussion: Add new deleteObject functions to Persistence API
The 'revision' parameter was added in https://github.com/apache/pekko/commit/66afe3fefb99dbc3e9e34755f28f4811b7624e86#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, 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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913778
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
GitHub user raboof added a comment to the discussion: Add new deleteObject functions to Persistence API
looks like `upsertObject` just fails with an `IllegalStateException`, so that might be fine - though a new Exception class might be slightly nicer?
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913997
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
@Roiocam @He-Pin https://github.com/apache/pekko-persistence-jdbc/pull/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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-9104660
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913648
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
GitHub user mdedetrich added a comment to the discussion: Add new deleteObject functions to Persistence API
Let me know if I am missing something but adding a new `deleteObject` API to Pekko core with a default implementation of throwing a "not supported" type of exception seems perfectly reasonable for me.
Regarding number 2, I recommended returning a failed future but you state that this is not a good approach because of the "let it crash" mentality? If so I don't know what to do here per say, tbh I don't have much domain knowledge here. It makes sense to have this new behaviour in a new set of API functions as well, might be a bit messy but this can then all be cleaned up in Pekko 2
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8911589
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
GitHub user raboof added a comment to the discussion: Add new deleteObject functions to Persistence API
> 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 is already deleted.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913848
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
For me, if we start throwing exceptions when now rows are deleted - that is a big behaviour change. Existing v1.0 code ignores it. No user would expect us to change this in a minor release. New function needed if we seriously want to change this.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8894056
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
GitHub user mdedetrich added a comment to the discussion: Add new deleteObject functions to Persistence API
> But as far as PR#[apache/pekko-persistence-jdbc#156](https://github.com/apache/pekko-persistence-jdbc/pull/156) is concerned, it also has a different behavior of delete in 1.0. Once the PR is merged, it will cause the possibility that the user expects to delete but does not delete.
Just wanted to point out that changing the behaviour is not out of the question as long as its warranted and ideally the breaking behaviour is more strict then the current behaviour so its easy to detect, i.e. Pekko 1.1.x [has a breaking change ](https://pekko.apache.org/docs/pekko/snapshot/migration/migration-guide-1.0.x-1.1.x.html#splitwhen-splitafter-behavior-change)
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8907818
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
GitHub user mdedetrich added a comment to the discussion: Add new deleteObject functions to Persistence API
> 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
```scala
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.
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
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8913583
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning edited a comment on the discussion: Add new deleteObject functions to Persistence API
## 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
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8975620
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
GitHub user He-Pin added a comment to the discussion: Add new deleteObject functions to Persistence API
As point 3,we should make the dependency just as the same, so add a new dependency is a no, sorry not noticed that.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-9104988
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
This is already the case with the `deleteObject(persistenceId)` function - the deprecated function - it returns Done if the object is already deleted.
I will continue to argue that if you want to notify the user that no row was deleted we need a new set of API functions.
If this was the first Persistence API release, I would agree to change this but we can't go and chance it after the fact. We need to maintain compatibility.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8900014
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
GitHub user Roiocam added a comment to the discussion: Add new deleteObject functions to Persistence API
**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](https://github.com/apache/pekko/blob/fbd36f4b33ffa762030436efe158ec94c6e4b050/persistence-typed/src/main/scala/org/apache/pekko/persistence/typed/state/scaladsl/Effect.scala#L43), 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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8893915
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
To be clear, I don't support an API change either. I am getting a lot of suggestions to make API breaking changes in https://github.com/apache/pekko-persistence-jdbc/pull/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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8894000
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [D] Add new deleteObject functions to Persistence API [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
GitHub user pjfanning added a comment to the discussion: Add new deleteObject functions to Persistence API
@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.
GitHub link: https://github.com/apache/pekko/discussions/1233#discussioncomment-8905701
----
This is an automatically sent email for notifications@pekko.apache.org.
To unsubscribe, please send an email to: notifications-unsubscribe@pekko.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org