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/19 13:27:24 UTC
[PR] take revision value into account when deleting [incubator-pekko-persistence-jdbc]
pjfanning opened a new pull request, #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156
relates to #30
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536787088
##########
core/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))
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531732671
##########
core/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))
Review Comment:
changing a public API method/function signature is not allowed - we would need a new API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536783376
##########
core/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))
Review Comment:
I don't understand - the point is we have a common Persistence API - why would we hack in extra methods in just this implementation? Should we update the common Persistence API and uptake it in all the persistence implementations?
##########
core/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))
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536782900
##########
core/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))
Review Comment:
That can be done in another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1576113783
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +114,40 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "fail to delete old object revision" in {
+ val f = for {
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ } yield d
+ whenReady(f.failed) { e =>
+ e shouldBe a[DurableStateStoreException]
Review Comment:
test rewritten but needs a bit more manual testing from me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1577103909
##########
docs/src/main/paradox/configuration.md:
##########
@@ -145,4 +145,4 @@ pekko-persistence-jdbc {
## Explicitly shutting down the database connections
The plugin automatically shuts down the HikariCP connection pool when the ActorSystem is terminated.
-This is done using @apidoc[ActorSystem.registerOnTermination](ActorSystem).
+This is done using @apidoc[org.apache.pekko.actor.ActorSystem.registerOnTermination](org.apache.pekko.actor.ActorSystem).
Review Comment:
This shouldn't be a problem, if it was, we could optimize it as:
```suggestion
This is done using @apidoc[ActorSystem.registerOnTermination](org.apache.pekko.actor.ActorSystem).
```
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -46,6 +47,9 @@ import slick.jdbc.{ H2Profile, JdbcProfile, OracleProfile, PostgresProfile, SQLS
private[jdbc] def selectFromDbByPersistenceId(persistenceId: Rep[String]) =
durableStateTable.filter(_.persistenceId === persistenceId)
+ private[jdbc] def selectFromDbByPersistenceId(persistenceId: String) =
+ durableStateTable.filter(_.persistenceId === persistenceId)
Review Comment:
I think this is duplicate, if you want to add a method that is used from outside, you may want to implement it like this:(i didn't verify it)
```
def selectFromDbByPersistenceId(persistenceId: String) =
db.run(durableStateTable.filter(_.persistenceId === persistenceId).result)
```
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala:
##########
@@ -113,21 +112,31 @@ class JdbcDurableStateStore[A](
}
}
- def deleteObject(persistenceId: String): Future[Done] =
+ override def deleteObject(persistenceId: String): Future[Done] =
db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
- def deleteObject(persistenceId: String, revision: Long): Future[Done] =
- 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) {
Review Comment:
Maybe `if (count != 1)`? we need to consider the case that we delete more than one.
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -92,6 +96,16 @@ import slick.jdbc.{ H2Profile, JdbcProfile, OracleProfile, PostgresProfile, SQLS
durableStateTable.filter(_.persistenceId === persistenceId).delete
}
+ /**
+ * Deletes a particular revision of an object based on its persistenceId.
+ * This revision may no longer exist and if so, no delete will occur.
+ *
+ * @since 1.1.0
+ */
+ private[jdbc] def deleteBasedOnPersistenceIdAndRevision(persistenceId: String, revision: Long) = {
+ durableStateTable.filter(r => r.persistenceId === persistenceId && r.revision === revision).delete
Review Comment:
we could use `selectFromDbByPersistenceId(persistenceId).filter(_. revision === revision).delete`. filter mutiple columns in one line wasn't recommended? at least i didn't see any usage in the official doc: https://scala-slick.org/doc/stable/queries.html#sorting-and-filtering
the parameter should be Rep[String] too because you are defining a SQL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1530420546
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -54,7 +54,7 @@ import pekko.persistence.jdbc.config.DurableStateTableConfiguration
}
private[jdbc] def selectFromDbByPersistenceId(persistenceId: Rep[String]) =
- durableStateTable.filter(_.persistenceId === persistenceId)
+ durableStateTable.filter(_.persistenceId === persistenceId).sortBy(_.revision.desc)
Review Comment:
it seems to me that we should sort this so that the result is based on the revision number - not sure why this is not already the case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536786162
##########
core/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))
Review Comment:
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536784037
##########
core/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))
Review Comment:
I created https://github.com/apache/pekko/issues/1232 - is 1232 worth delaying the 1.1.0-M1 release for? I don't think so, myself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1575378886
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +114,40 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "fail to delete old object revision" in {
+ val f = for {
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ } yield d
+ whenReady(f.failed) { e =>
+ e shouldBe a[DurableStateStoreException]
Review Comment:
WIP here - I still need to rewrite this test to make the correct assertions depending on whether pekko-persistence 1.0.x is used or 1.1.x is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531448077
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +113,45 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "delete old object revision but not latest" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p987")
+
+ } yield h
+ } { v =>
+ v.value shouldBe Some("updated valid string")
+ }
+ }
+ "delete latest object revision but not older one" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p9876", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p9876")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p9876", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p9876", 2)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p9876")
+
+ } yield h
+ } { v =>
+ // TODO current behavior is that deleting the latest revision means getObject returns None (not an older revision)
+ v.value shouldBe None
Review Comment:
> So far, it seems like we don't actually store the various revisions for the pekko-persistence-jdbc module.
That's why they named it durable state rather than event sourcing, this behavior not a issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1530423058
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +113,45 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "delete old object revision but not latest" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p987")
+
+ } yield h
+ } { v =>
+ v.value shouldBe Some("updated valid string")
+ }
+ }
+ "delete latest object revision but not older one" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p9876", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p9876")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p9876", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p9876", 2)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p9876")
+
+ } yield h
+ } { v =>
+ // TODO current behavior is that deleting the latest revision means getObject returns None (not an older revision)
+ v.value shouldBe None
Review Comment:
I don't think this result is correct but I'm making the result match the observed behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1576582556
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala:
##########
@@ -113,21 +112,34 @@ class JdbcDurableStateStore[A](
}
}
- def deleteObject(persistenceId: String): Future[Done] =
+ override def deleteObject(persistenceId: String): Future[Done] =
db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
- def deleteObject(persistenceId: String, revision: Long): Future[Done] =
- db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
+ override def deleteObject(persistenceId: String, revision: Long): Future[Done] = {
+ db.run(queries.deleteBasedOnPersistenceIdAndRevision(persistenceId, revision)).map { count =>
+ {
Review Comment:
Use transformWith？
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala:
##########
@@ -113,21 +112,34 @@ class JdbcDurableStateStore[A](
}
}
- def deleteObject(persistenceId: String): Future[Done] =
+ override def deleteObject(persistenceId: String): Future[Done] =
db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
- def deleteObject(persistenceId: String, revision: Long): Future[Done] =
- 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) {
+ // if you run this code with Pekko 1.0.x, no exception will be thrown here
+ // this matches the behavior of pekko-connectors-jdbc 1.0.x
+ // if you run this code with Pekko 1.1.x, a DeleteRevisionException will be thrown here
+ DurableStateExceptionSupport.createDeleteRevisionExceptionIfSupported(
+ s"Failed to delete object with persistenceId [$persistenceId] and revision [$revision]")
+ .foreach(throw _)
+ }
+ Done
+ }
+ }
Review Comment:
Set the execution context to be samethread
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.jdbc.state.scaladsl
+
+import java.lang.invoke.{ MethodHandles, MethodType }
+
+import scala.util.Try
+
+/**
+ * INTERNAL API
+ *
+ * Support for creating a `DeleteRevisionException`if the class is
+ * available on the classpath. Pekko 1.0 does not have this class, but
+ * it is added in Pekko 1.1.
+ */
+private[scaladsl] object DurableStateExceptionSupport {
+ val DeleteRevisionExceptionClass =
+ "org.apache.pekko.persistence.state.exception.DeleteRevisionException"
+ private val methodHandleLookup = MethodHandles.publicLookup()
Review Comment:
Lookup is only be used once
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.jdbc.state.scaladsl
+
+import java.lang.invoke.{ MethodHandles, MethodType }
+
+import scala.util.Try
+
+/**
+ * INTERNAL API
+ *
+ * Support for creating a `DeleteRevisionException`if the class is
+ * available on the classpath. Pekko 1.0 does not have this class, but
+ * it is added in Pekko 1.1.
+ */
+private[scaladsl] object DurableStateExceptionSupport {
+ val DeleteRevisionExceptionClass =
+ "org.apache.pekko.persistence.state.exception.DeleteRevisionException"
+ private val methodHandleLookup = MethodHandles.publicLookup()
+
+ private def exceptionClassOpt: Option[Class[_]] =
+ Try(Class.forName(DeleteRevisionExceptionClass)).toOption
+
+ private lazy val constructorOpt = exceptionClassOpt.map { clz =>
+ val mt = MethodType.methodType(classOf[Unit], classOf[String])
+ methodHandleLookup.findConstructor(clz, mt)
+ }
+
+ def createDeleteRevisionExceptionIfSupported(message: String): Option[Exception] =
+ constructorOpt.map { constructor =>
+ constructor.invoke(message).asInstanceOf[Exception]
+ }
+
Review Comment:
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.
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.jdbc.state.scaladsl
+
+import java.lang.invoke.{ MethodHandles, MethodType }
+
+import scala.util.Try
+
+/**
+ * INTERNAL API
+ *
+ * Support for creating a `DeleteRevisionException`if the class is
+ * available on the classpath. Pekko 1.0 does not have this class, but
+ * it is added in Pekko 1.1.
+ */
+private[scaladsl] object DurableStateExceptionSupport {
+ val DeleteRevisionExceptionClass =
+ "org.apache.pekko.persistence.state.exception.DeleteRevisionException"
+ private val methodHandleLookup = MethodHandles.publicLookup()
+
+ private def exceptionClassOpt: Option[Class[_]] =
+ Try(Class.forName(DeleteRevisionExceptionClass)).toOption
Review Comment:
I think this private method is not needed, inline it to the lazy val?
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.jdbc.state.scaladsl
+
+import java.lang.invoke.{ MethodHandles, MethodType }
+
+import scala.util.Try
+
+/**
+ * INTERNAL API
+ *
+ * Support for creating a `DeleteRevisionException`if the class is
+ * available on the classpath. Pekko 1.0 does not have this class, but
+ * it is added in Pekko 1.1.
+ */
+private[scaladsl] object DurableStateExceptionSupport {
+ val DeleteRevisionExceptionClass =
Review Comment:
I would suggest mark it private , this will be visible in Java, seems the only usage is in testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1576670500
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.jdbc.state.scaladsl
+
+import java.lang.invoke.{ MethodHandles, MethodType }
+
+import scala.util.Try
+
+/**
+ * INTERNAL API
+ *
+ * Support for creating a `DeleteRevisionException`if the class is
+ * available on the classpath. Pekko 1.0 does not have this class, but
+ * it is added in Pekko 1.1.
+ */
+private[scaladsl] object DurableStateExceptionSupport {
+ val DeleteRevisionExceptionClass =
+ "org.apache.pekko.persistence.state.exception.DeleteRevisionException"
+ private val methodHandleLookup = MethodHandles.publicLookup()
+
+ private def exceptionClassOpt: Option[Class[_]] =
+ Try(Class.forName(DeleteRevisionExceptionClass)).toOption
Review Comment:
This is what hotspot compiler is for. I seriously wonder why we bother using Scala when reviewers expect assembly language like code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531754105
##########
core/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))
Review Comment:
> 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 a stretch to also break source compatibility however in this case it seems like we can avoid it
##########
core/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))
Review Comment:
> 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531749805
##########
core/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))
Review Comment:
the super class method is deprecated so this is too - no need to also deprecate this too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1530481197
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +113,45 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "delete old object revision but not latest" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p987")
+
+ } yield h
+ } { v =>
+ v.value shouldBe Some("updated valid string")
+ }
+ }
+ "delete latest object revision but not older one" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p9876", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p9876")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p9876", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p9876", 2)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p9876")
+
+ } yield h
+ } { v =>
+ // TODO current behavior is that deleting the latest revision means getObject returns None (not an older revision)
+ v.value shouldBe None
Review Comment:
@nvollmar You appear to be quite interested in the Cassandra persistence. I'm wondering if you know how the 'revision' is supported in that module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1577855282
##########
docs/src/main/paradox/configuration.md:
##########
@@ -145,4 +145,4 @@ pekko-persistence-jdbc {
## Explicitly shutting down the database connections
The plugin automatically shuts down the HikariCP connection pool when the ActorSystem is terminated.
-This is done using @apidoc[ActorSystem.registerOnTermination](ActorSystem).
+This is done using @apidoc[org.apache.pekko.actor.ActorSystem.registerOnTermination](org.apache.pekko.actor.ActorSystem).
Review Comment:
was causing an issue when I added a dependency on pekko-persistence-typed but that dependency is gone now so this is not needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531744677
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -54,7 +54,7 @@ import pekko.persistence.jdbc.config.DurableStateTableConfiguration
}
private[jdbc] def selectFromDbByPersistenceId(persistenceId: Rep[String]) =
- durableStateTable.filter(_.persistenceId === persistenceId)
+ durableStateTable.filter(_.persistenceId === persistenceId).sortBy(_.revision.desc)
Review Comment:
removing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1530453336
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +113,45 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "delete old object revision but not latest" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p987")
+
+ } yield h
+ } { v =>
+ v.value shouldBe Some("updated valid string")
+ }
+ }
+ "delete latest object revision but not older one" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p9876", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p9876")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p9876", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p9876", 2)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p9876")
+
+ } yield h
+ } { v =>
+ // TODO current behavior is that deleting the latest revision means getObject returns None (not an older revision)
+ v.value shouldBe None
Review Comment:
So far, it seems like we don't actually store the various revisions for the pekko-persistence-jdbc module.
It seems like only the most recently added one is kept. So if you delete an old revision, nothing happens (nothing to delete). If you delete the latest revision, then there is nothing left with that persistenceId.
I will debug this more thoroughly later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536778850
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -102,6 +102,13 @@ import pekko.persistence.jdbc.config.DurableStateTableConfiguration
durableStateTable.filter(_.persistenceId === persistenceId).delete
}
+ /**
+ * @since 1.1.0
+ */
Review Comment:
add doc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1576761429
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.jdbc.state.scaladsl
+
+import java.lang.invoke.{ MethodHandles, MethodType }
+
+import scala.util.Try
+
+/**
+ * INTERNAL API
+ *
+ * Support for creating a `DeleteRevisionException`if the class is
+ * available on the classpath. Pekko 1.0 does not have this class, but
+ * it is added in Pekko 1.1.
+ */
+private[scaladsl] object DurableStateExceptionSupport {
+ val DeleteRevisionExceptionClass =
+ "org.apache.pekko.persistence.state.exception.DeleteRevisionException"
+ private val methodHandleLookup = MethodHandles.publicLookup()
+
+ private def exceptionClassOpt: Option[Class[_]] =
+ Try(Class.forName(DeleteRevisionExceptionClass)).toOption
+
+ private lazy val constructorOpt = exceptionClassOpt.map { clz =>
+ val mt = MethodType.methodType(classOf[Unit], classOf[String])
+ methodHandleLookup.findConstructor(clz, mt)
+ }
+
+ def createDeleteRevisionExceptionIfSupported(message: String): Option[Exception] =
+ constructorOpt.map { constructor =>
+ constructor.invoke(message).asInstanceOf[Exception]
+ }
+
Review Comment:
I prefer the current style in my PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531750648
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +113,45 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "delete old object revision but not latest" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p987")
+
+ } yield h
+ } { v =>
+ v.value shouldBe Some("updated valid string")
+ }
+ }
+ "delete latest object revision but not older one" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p9876", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p9876")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p9876", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p9876", 2)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p9876")
+
+ } yield h
+ } { v =>
+ // TODO current behavior is that deleting the latest revision means getObject returns None (not an older revision)
+ v.value shouldBe None
Review Comment:
thanks for the explanation @Roiocam
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting [incubator-pekko-persistence-jdbc]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#issuecomment-2007820611
Unfortunately I don't have much experience with this but I will look into it tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1538799495
##########
core/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))
Review Comment:
if DAO doesn't delete anything, throwing an exception is reasonable for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#issuecomment-2072087823
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1576760872
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/DurableStateExceptionSupport.scala:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.jdbc.state.scaladsl
+
+import java.lang.invoke.{ MethodHandles, MethodType }
+
+import scala.util.Try
+
+/**
+ * INTERNAL API
+ *
+ * Support for creating a `DeleteRevisionException`if the class is
+ * available on the classpath. Pekko 1.0 does not have this class, but
+ * it is added in Pekko 1.1.
+ */
+private[scaladsl] object DurableStateExceptionSupport {
+ val DeleteRevisionExceptionClass =
Review Comment:
I don't see any massive benefit to making this private
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1577821154
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -92,6 +96,16 @@ import slick.jdbc.{ H2Profile, JdbcProfile, OracleProfile, PostgresProfile, SQLS
durableStateTable.filter(_.persistenceId === persistenceId).delete
}
+ /**
+ * Deletes a particular revision of an object based on its persistenceId.
+ * This revision may no longer exist and if so, no delete will occur.
+ *
+ * @since 1.1.0
+ */
+ private[jdbc] def deleteBasedOnPersistenceIdAndRevision(persistenceId: String, revision: Long) = {
+ durableStateTable.filter(r => r.persistenceId === persistenceId && r.revision === revision).delete
Review Comment:
I've used filters with multiple conditions many times with no issues. I can change this nonetheless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1577857396
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -46,6 +47,9 @@ import slick.jdbc.{ H2Profile, JdbcProfile, OracleProfile, PostgresProfile, SQLS
private[jdbc] def selectFromDbByPersistenceId(persistenceId: Rep[String]) =
durableStateTable.filter(_.persistenceId === persistenceId)
+ private[jdbc] def selectFromDbByPersistenceId(persistenceId: String) =
+ durableStateTable.filter(_.persistenceId === persistenceId)
Review Comment:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531735757
##########
core/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))
Review Comment:
this API overrides a super class functions so it can't change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531243218
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +113,45 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "delete old object revision but not latest" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p987")
+
+ } yield h
+ } { v =>
+ v.value shouldBe Some("updated valid string")
+ }
+ }
+ "delete latest object revision but not older one" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p9876", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p9876")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p9876", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p9876", 2)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p9876")
+
+ } yield h
+ } { v =>
+ // TODO current behavior is that deleting the latest revision means getObject returns None (not an older revision)
+ v.value shouldBe None
Review Comment:
so the 'upsert logic' only updates the existing record if revision > 1
https://github.com/apache/incubator-pekko-persistence-jdbc/blob/7cbcb3f40641a145d1ae5cfbe5ec1e1fef61135a/core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala#L105
I don't understand why the API has deleteObject(persistenceId, revision) if it does not keep all the revisions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531450564
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +113,45 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "delete old object revision but not latest" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p987")
+
+ } yield h
+ } { v =>
+ v.value shouldBe Some("updated valid string")
+ }
+ }
+ "delete latest object revision but not older one" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p9876", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p9876")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p9876", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p9876", 2)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p9876")
+
+ } yield h
+ } { v =>
+ // TODO current behavior is that deleting the latest revision means getObject returns None (not an older revision)
+ v.value shouldBe None
Review Comment:
> I don't understand why the API has deleteObject(persistenceId, revision) if it does not keep all the revisions.
I think it is for safety reasons. When a split-brain occurs, nodes in the older version of the partition are not allowed to try to delete the data with the newer version.
For example, we have nodes: [A, B, C, D], and the split-brain splits them into two network partitions of [A,C] [B,D].
Node A updated the revision of persistence Actor X001 from 10 to 11, while Node B has Sharding X001 is still version 10, so it is not allowed to delete X001 from B, but it is okay to delete X001 from A.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536776215
##########
core/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))
Review Comment:
this function is part of the shared Persistence API - ie it overrides a function in a super class
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536779123
##########
core/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))
Review Comment:
Better we add a new API for this , which returns the detailed result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1536781784
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -102,6 +102,13 @@ import pekko.persistence.jdbc.config.DurableStateTableConfiguration
durableStateTable.filter(_.persistenceId === persistenceId).delete
}
+ /**
+ * @since 1.1.0
+ */
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [incubator-pekko-persistence-jdbc]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1531452381
##########
core/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))
Review Comment:
Can you add a `deprecated` comment here？
##########
core/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))
Review Comment:
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]
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/DurableStateQueries.scala:
##########
@@ -54,7 +54,7 @@ import pekko.persistence.jdbc.config.DurableStateTableConfiguration
}
private[jdbc] def selectFromDbByPersistenceId(persistenceId: Rep[String]) =
- durableStateTable.filter(_.persistenceId === persistenceId)
+ durableStateTable.filter(_.persistenceId === persistenceId).sortBy(_.revision.desc)
Review Comment:
Ordering is no necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting [incubator-pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/incubator-pekko-persistence-jdbc/pull/156#discussion_r1530459914
##########
core/src/test/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateSpec.scala:
##########
@@ -113,6 +113,45 @@ abstract class JdbcDurableStateSpec(config: Config, schemaType: SchemaType) exte
}
}
}
+ "delete old object revision but not latest" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p987", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p987")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p987", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p987", 1)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p987")
+
+ } yield h
+ } { v =>
+ v.value shouldBe Some("updated valid string")
+ }
+ }
+ "delete latest object revision but not older one" in {
+ whenReady {
+ for {
+
+ n <- stateStoreString.upsertObject("p9876", 1, "a valid string", "t123")
+ _ = n shouldBe pekko.Done
+ g <- stateStoreString.getObject("p9876")
+ _ = g.value shouldBe Some("a valid string")
+ u <- stateStoreString.upsertObject("p9876", 2, "updated valid string", "t123")
+ _ = u shouldBe pekko.Done
+ d <- stateStoreString.deleteObject("p9876", 2)
+ _ = d shouldBe pekko.Done
+ h <- stateStoreString.getObject("p9876")
+
+ } yield h
+ } { v =>
+ // TODO current behavior is that deleting the latest revision means getObject returns None (not an older revision)
+ v.value shouldBe None
Review Comment:
@mdedetrich @Roiocam I don't want to off in the wrong direction. Could you review the existing changes and check out my preliminary analysis of why this test doesn't work as I would expect it to?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1576795445
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala:
##########
@@ -113,21 +112,34 @@ class JdbcDurableStateStore[A](
}
}
- def deleteObject(persistenceId: String): Future[Done] =
+ override def deleteObject(persistenceId: String): Future[Done] =
db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
- def deleteObject(persistenceId: String, revision: Long): Future[Done] =
- 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) {
+ // if you run this code with Pekko 1.0.x, no exception will be thrown here
+ // this matches the behavior of pekko-connectors-jdbc 1.0.x
+ // if you run this code with Pekko 1.1.x, a DeleteRevisionException will be thrown here
+ DurableStateExceptionSupport.createDeleteRevisionExceptionIfSupported(
+ s"Failed to delete object with persistenceId [$persistenceId] and revision [$revision]")
+ .foreach(throw _)
+ }
+ Done
+ }
+ }
Review Comment:
changed to use parasitic context
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1577856910
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala:
##########
@@ -113,21 +112,31 @@ class JdbcDurableStateStore[A](
}
}
- def deleteObject(persistenceId: String): Future[Done] =
+ override def deleteObject(persistenceId: String): Future[Done] =
db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
- def deleteObject(persistenceId: String, revision: Long): Future[Done] =
- 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) {
Review Comment:
changed - I added logic to output a different message if the count > 1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] take revision value into account when deleting DurableState [pekko-persistence-jdbc]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #156:
URL: https://github.com/apache/pekko-persistence-jdbc/pull/156#discussion_r1576775456
##########
core/src/main/scala/org/apache/pekko/persistence/jdbc/state/scaladsl/JdbcDurableStateStore.scala:
##########
@@ -113,21 +112,34 @@ class JdbcDurableStateStore[A](
}
}
- def deleteObject(persistenceId: String): Future[Done] =
+ override def deleteObject(persistenceId: String): Future[Done] =
db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
- def deleteObject(persistenceId: String, revision: Long): Future[Done] =
- db.run(queries.deleteFromDb(persistenceId).map(_ => Done))
+ override def deleteObject(persistenceId: String, revision: Long): Future[Done] = {
+ db.run(queries.deleteBasedOnPersistenceIdAndRevision(persistenceId, revision)).map { count =>
+ {
Review Comment:
transformWith lets you convert Try[T] to a new result? Why do I need to convert the failure case here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org