take revision value into account when deleting DurableState #156

pjfanning · 2024-03-19T13:27:22Z

relates to #30

I have gone back to experimenting a bit - so this is not mergeable right now

core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala

core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala

mdedetrich · 2024-03-19T18:04:06Z

Unfortunately I don't have much experience with this but I will look into it tomorrow.

Roiocam

LGTM until these have been fixed.

core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala

.../src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala

Roiocam · 2024-03-20T02:39:15Z

.../src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala

@@ -117,7 +117,7 @@ class JdbcDurableStateStore[A](
    db.run(queries.deleteFromDb(persistenceId).map(_ => Done))

  def deleteObject(persistenceId: String, revision: Long): Future[Done] =
-    db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
+    db.run(queries.deleteBasedOnPersistenceIdAndRevision(persistenceId, revision).map(_ => Done))


If the deletion operation is not allowed, there will be no warnings and exceptions. I think we should notify users in some way, such as returning Future[Boolean]

changing a public API method/function signature is not allowed - we would need a new API

this API overrides a super class functions so it can't change

changing a public API method/function signature is not allowed - we would need a new API

We have already broken ABI because of Slick 3.5.0 and this project doesn't guarantee SemVer so its not a stretch to also break source compatibility however in this case it seems like we can avoid it

this function is part of the shared Persistence API - ie it overrides a function in a super class

I don't understand - the point is we have a common Persistence API - why would we hack in extra methods in just this implementation? Shouldn't we update the common Persistence API and uptake it in all the persistence implementations?

I created apache/pekko#1232 - is 1232 worth delaying the 1.1.0-M1 release for? I don't think so, myself.

If the deletion operation is not allowed, there will be no warnings and exceptions. I think we should notify users in some way, such as returning Future[Boolean]

Isn't a better way that doesn't break binary compatibility to just return a Future.failed with some sealed exception trait so users can get more information?

can this be done in another PR? - it does not just affect the function that I am working on - it affects a number of other functions

It also looks like a change that we would want to replicate across all our persistence implementations. Some of them might already have Future failure results for failures to delete but it's quite likely that they all quietly ignore the fact that delete call deleted nothing.

I really think this is a set of changes in behaviour that should be discussed publicly before we make them.

if DAO doesn't delete anything, throwing an exception is reasonable for me.

core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala

He-Pin

lgtm, just need update the doc.

core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala

add test add tests scalafmt update test Update DurableStateQueries.scala scalafmt

pjfanning · 2024-04-23T11:49:37Z

I still need to double check the tests pass with Pekko 1.1 but the code is more or less ready (I think).
Would anyone have time to review the latest position?

@mdedetrich @He-Pin @Roiocam @nvollmar @samueleresca @raboof

He-Pin

Overall lgtm, reviewed from my phone, sorry for not properly highlighted

He-Pin · 2024-04-23T16:46:31Z

...in/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala

+  private val methodHandleLookup = MethodHandles.publicLookup()
+
+  private def exceptionClassOpt: Option[Class[_]] =
+    Try(Class.forName(DeleteRevisionExceptionClass)).toOption


I think this private method is not needed, inline it to the lazy val?

This is what hotspot compiler is for. I seriously wonder why we bother using Scala when reviewers expect assembly language like code.

He-Pin · 2024-04-23T16:49:48Z

...in/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala

+private[scaladsl] object DurableStateExceptionSupport {
+  val DeleteRevisionExceptionClass =
+    "org.apache.pekko.persistence.state.exception.DeleteRevisionException"
+  private val methodHandleLookup = MethodHandles.publicLookup()


Lookup is only be used once

He-Pin · 2024-04-23T16:52:31Z

...in/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala

+  * it is added in Pekko 1.1.
+  */
+private[scaladsl] object DurableStateExceptionSupport {
+  val DeleteRevisionExceptionClass =


I would suggest mark it private , this will be visible in Java, seems the only usage is in testing

I don't see any massive benefit to making this private

He-Pin · 2024-04-23T16:55:12Z

.../src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala

-    db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
+  override def deleteObject(persistenceId: String, revision: Long): Future[Done] = {
+    db.run(queries.deleteBasedOnPersistenceIdAndRevision(persistenceId, revision)).map { count =>
+      {


Use transformWith？

transformWith lets you convert Try[T] to a new result? Why do I need to convert the failure case here?

He-Pin · 2024-04-23T16:55:55Z

.../src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala

+        }
+        Done
+      }
+    }


Set the execution context to be samethread

changed to use parasitic context

He-Pin · 2024-04-23T16:58:26Z

...in/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala

+    constructorOpt.map { constructor =>
+      constructor.invoke(message).asInstanceOf[Exception]
+    }
+


I think we can just let this method returns Done, and if it's 1.1.x and count is 0，throw exception, this will be simpler and the compiler is happy too.

I prefer the current style in my PR.

Roiocam

LGTM overall, leave some slick optmization suggest.

core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala

Roiocam · 2024-04-24T01:43:45Z

.../src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala

-    db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
+  override def deleteObject(persistenceId: String, revision: Long): Future[Done] =
+    db.run(queries.deleteBasedOnPersistenceIdAndRevision(persistenceId, revision)).map { count =>
+      if (count == 0) {


Maybe if (count != 1)? we need to consider the case that we delete more than one.

changed - I added logic to output a different message if the count > 1

docs/src/main/paradox/configuration.md

Roiocam

LGTM

pjfanning marked this pull request as draft March 19, 2024 13:27

pjfanning commented Mar 19, 2024

View reviewed changes

core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala Outdated Show resolved Hide resolved

pjfanning commented Mar 19, 2024

View reviewed changes

core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala Show resolved Hide resolved

pjfanning changed the title ~~take revision value into account when deleting~~ take revision value into account when deleting DurableState Mar 19, 2024

Roiocam reviewed Mar 20, 2024

View reviewed changes

pjfanning marked this pull request as ready for review March 20, 2024 09:31

pjfanning requested review from raboof, He-Pin, nvollmar and samueleresca March 24, 2024 09:36

He-Pin reviewed Mar 24, 2024

View reviewed changes

core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala Show resolved Hide resolved

He-Pin approved these changes Mar 24, 2024

View reviewed changes

pjfanning mentioned this pull request Mar 24, 2024

Add new deleteObject functions to Persistence API apache/pekko#1232

Closed

pjfanning marked this pull request as draft March 27, 2024 09:43

pjfanning force-pushed the delete-revision branch from 88b2c66 to 25eeb7a Compare April 13, 2024 13:08

pjfanning commented Apr 22, 2024

View reviewed changes

core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala Outdated Show resolved Hide resolved

pjfanning added 10 commits April 22, 2024 23:36

take revision value into account when deleting

991b5bb

add test add tests scalafmt update test Update DurableStateQueries.scala scalafmt

temporarily throw DurableStateStoreException

e32a769

Update JdbcDurableStateStore.scala

5a66668

rework exceptions

200a75b

Update configuration.md

b065e38

Update configuration.md

36c462c

Update JdbcDurableStateSpec.scala

eb67228

uptake apache/pekko#1271

ac5ceff

scalafmt

87265e5

Update JdbcDurableStateSpec.scala

15bda57

pjfanning force-pushed the delete-revision branch from 43d362f to 15bda57 Compare April 22, 2024 21:41

try to fix test

590f7dd

pjfanning requested a review from He-Pin April 23, 2024 11:47

Update DurableStateExceptionSupport.scala

9884284

He-Pin approved these changes Apr 23, 2024

View reviewed changes

pjfanning added 2 commits April 23, 2024 20:57

refactor

3e8142a

use parasitic context

cdd0d2c

Roiocam reviewed Apr 24, 2024

View reviewed changes

pjfanning added 5 commits April 24, 2024 14:45

apply review suggestions

b594b71

review change

8773851

Update JdbcDurableStateStore.scala

7e2c17f

revert doc change

ecb567a

Update JdbcDurableStateStore.scala

7c8f830

Roiocam approved these changes Apr 24, 2024

View reviewed changes

pjfanning marked this pull request as ready for review April 25, 2024 20:02

pjfanning merged commit 08db474 into apache:main Apr 25, 2024
20 checks passed

pjfanning deleted the delete-revision branch April 25, 2024 20:02

pjfanning added this to the 1.1.0-M1 milestone Apr 25, 2024

pjfanning mentioned this pull request Apr 25, 2024

Implement deleteObject(persistenceId: String, revision: Long) properly #30

Closed

pjfanning modified the milestones: 1.1.0-M1-RC1, 1.1.0-M1 May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

take revision value into account when deleting DurableState #156

take revision value into account when deleting DurableState #156

pjfanning commented Mar 19, 2024 •

edited

Loading

mdedetrich commented Mar 19, 2024

Roiocam left a comment

Roiocam Mar 20, 2024

pjfanning Mar 20, 2024

pjfanning Mar 20, 2024

mdedetrich Mar 20, 2024 •

edited

Loading

pjfanning Mar 24, 2024

pjfanning Mar 24, 2024 •

edited

Loading

pjfanning Mar 24, 2024

mdedetrich Mar 24, 2024

pjfanning Mar 24, 2024

Roiocam Mar 26, 2024

He-Pin left a comment

pjfanning commented Apr 23, 2024

He-Pin left a comment

He-Pin Apr 23, 2024

pjfanning Apr 23, 2024

He-Pin Apr 23, 2024

He-Pin Apr 23, 2024

pjfanning Apr 23, 2024

He-Pin Apr 23, 2024

pjfanning Apr 23, 2024

He-Pin Apr 23, 2024

pjfanning Apr 23, 2024

He-Pin Apr 23, 2024

pjfanning Apr 23, 2024

Roiocam left a comment

Roiocam Apr 24, 2024

pjfanning Apr 24, 2024

Roiocam left a comment

take revision value into account when deleting DurableState #156

take revision value into account when deleting DurableState #156

Conversation

pjfanning commented Mar 19, 2024 • edited Loading

mdedetrich commented Mar 19, 2024

Roiocam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdedetrich Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjfanning Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

He-Pin left a comment

Choose a reason for hiding this comment

pjfanning commented Apr 23, 2024

He-Pin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roiocam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roiocam left a comment

Choose a reason for hiding this comment

pjfanning commented Mar 19, 2024 •

edited

Loading

mdedetrich Mar 20, 2024 •

edited

Loading

pjfanning Mar 24, 2024 •

edited

Loading