-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for querying last known sequence number by persistenceId. #267
Changes from 2 commits
b4b4308
49b7262
b033218
f0cc6c5
ce15d5e
9609a8f
02197f8
711145f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* license agreements; and to You under the Apache License, version 2.0: | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* This file is part of the Apache Pekko project, which was derived from Akka. | ||
*/ | ||
|
||
package org.apache.pekko.persistence.jdbc.query.javadsl | ||
|
||
import java.util.Optional | ||
import java.util.concurrent.CompletionStage | ||
|
||
/** | ||
* A trait that enables querying the current last known sequence number for a given `persistenceId`. | ||
*/ | ||
trait CurrentLastKnownSequenceNumberByPersistenceIdQuery { | ||
|
||
/** | ||
* Returns the last known sequence number for the given `persistenceId`. Empty if the `persistenceId` is unknown. | ||
* | ||
* @param persistenceId The `persistenceId` for which the last known sequence number should be returned. | ||
* @return Some sequence number or None if the `persistenceId` is unknown. | ||
*/ | ||
def currentLastKnownSequenceNumberByPersistenceId(persistenceId: String): CompletionStage[Optional[Long]] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,11 +15,17 @@ | |
package org.apache.pekko.persistence.jdbc.query.javadsl | ||
|
||
import org.apache.pekko | ||
|
||
import java.util.Optional | ||
import java.util.concurrent.CompletionStage | ||
|
||
import pekko.NotUsed | ||
import pekko.persistence.jdbc.query.scaladsl.{ JdbcReadJournal => ScalaJdbcReadJournal } | ||
import pekko.persistence.query.{ EventEnvelope, Offset } | ||
import pekko.persistence.query.javadsl._ | ||
import pekko.stream.javadsl.Source | ||
import pekko.util.FutureConverters._ | ||
import pekko.util.OptionConverters._ | ||
|
||
object JdbcReadJournal { | ||
final val Identifier = ScalaJdbcReadJournal.Identifier | ||
|
@@ -32,7 +38,8 @@ class JdbcReadJournal(journal: ScalaJdbcReadJournal) | |
with CurrentEventsByPersistenceIdQuery | ||
with EventsByPersistenceIdQuery | ||
with CurrentEventsByTagQuery | ||
with EventsByTagQuery { | ||
with EventsByTagQuery | ||
with CurrentLastKnownSequenceNumberByPersistenceIdQuery { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need the CurrentLastKnownSequenceNumberByPersistenceIdQuery trait? Can't we just add the function without having a marker trait. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming a marker trait would allow writing this:
Instead of having to write this:
But if the goal is to eventually have the marker trait in the core repository then this serves very little and temporary purpose, so overall I agree with suggestion to remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Main intent was to stick to existing marker traits application + having that part of contract testable on its own. In our applications we rarely declare dependencies to the full query journal, usually we tend to have the capabilities speaking: object FancyAggregator {
def apply(readJournal: EventsByTagQuery)
} vs object FancyAggregator {
def apply(readJournal: JdbcReadJournal)
} If you feel more comfortable, surely, I can try to inline it without marker trait. Would make dependency injection a bit harder. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My aim would be merge this without the marker trait meaning that we could release a version pekko-persistence-jdbc 1.1.x that supports this but without needing a new Pekko core release. pekko-persistence 1.2.0-M2 and above could have the marker trait and a pekko-persistence-jdbc 1.2.0(-M1) release could rely on pekko-persistence 1.2.0-M2 and uptake the marker trait. I haven't studied pekko-persistence-r2dbc but I presume that it too could implement this support and in a similar timeline. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I will drop the marker trait today or tomorrow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed marker trait with ce15d5e. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ptrdom it looks like pekko-persistence-r2dbc can have a similar PR added at some stage? Can you confirm this? I can look into adding the marker trait to the pekko core but it could be a while before it makes it into a full release meaning that we will have to wait to add the uptake of the marker trait. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think it should be no problem to add this functionality to pekko-persistence-r2dbc. |
||
|
||
/** | ||
* Same type of query as `persistenceIds` but the event stream | ||
|
@@ -132,4 +139,10 @@ class JdbcReadJournal(journal: ScalaJdbcReadJournal) | |
*/ | ||
override def eventsByTag(tag: String, offset: Offset): Source[EventEnvelope, NotUsed] = | ||
journal.eventsByTag(tag, offset).asJava | ||
|
||
override def currentLastKnownSequenceNumberByPersistenceId(persistenceId: String): CompletionStage[Optional[Long]] = | ||
journal | ||
.currentLastKnownSequenceNumberByPersistenceId(persistenceId) | ||
.asJava | ||
.thenApply(_.toJava) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* license agreements; and to You under the Apache License, version 2.0: | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* This file is part of the Apache Pekko project, which was derived from Akka. | ||
*/ | ||
|
||
package org.apache.pekko.persistence.jdbc.query.scaladsl | ||
|
||
import scala.concurrent.Future | ||
|
||
/** | ||
* A trait that enables querying the current last known sequence number for a given `persistenceId`. | ||
*/ | ||
trait CurrentLastKnownSequenceNumberByPersistenceIdQuery { | ||
|
||
/** | ||
* Returns the last known sequence number for the given `persistenceId`. Empty if the `persistenceId` is unknown. | ||
* | ||
* @param persistenceId The `persistenceId` for which the last known sequence number should be returned. | ||
* @return Some sequence number or None if the `persistenceId` is unknown. | ||
*/ | ||
def currentLastKnownSequenceNumberByPersistenceId(persistenceId: String): Future[Option[Long]] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* license agreements; and to You under the Apache License, version 2.0: | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* This file is part of the Apache Pekko project, which was derived from Akka. | ||
*/ | ||
|
||
package org.apache.pekko.persistence.jdbc.query | ||
|
||
import org.scalatest.concurrent.ScalaFutures | ||
|
||
abstract class CurrentLastKnownSequenceNumberByPersistenceIdTest( | ||
config: String | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this style is still wrong There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hope f0cc6c5 does the job? |
||
) extends QueryTestSpec(config) with ScalaFutures { | ||
|
||
it should "return None for unknown persistenceId" in withActorSystem { implicit system => | ||
val journalOps = new ScalaJdbcReadJournalOperations(system) | ||
|
||
journalOps | ||
.currentLastKnownSequenceNumberByPersistenceId("unknown") | ||
.futureValue shouldBe None | ||
} | ||
|
||
it should "return last sequence number for known persistenceId" in withActorSystem { implicit system => | ||
val journalOps = new ScalaJdbcReadJournalOperations(system) | ||
|
||
withTestActors() { (actor1, _, _) => | ||
actor1 ! 1 | ||
actor1 ! 2 | ||
actor1 ! 3 | ||
actor1 ! 4 | ||
|
||
eventually { | ||
journalOps | ||
.currentLastKnownSequenceNumberByPersistenceId("my-1") | ||
.futureValue shouldBe Some(4) | ||
|
||
// Just ensuring that query targets the correct persistenceId. | ||
journalOps | ||
.currentLastKnownSequenceNumberByPersistenceId("my-2") | ||
.futureValue shouldBe None | ||
} | ||
} | ||
} | ||
} | ||
|
||
class H2ScalaCurrentLastKnownSequenceNumberByPersistenceIdTest | ||
extends CurrentLastKnownSequenceNumberByPersistenceIdTest("h2-shared-db-application.conf") | ||
with H2Cleaner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the license header on new files that are not copies of other files to be https://github.com/apache/pekko/blob/main/project/AddMetaInfLicenseFiles.scala#L1-L16 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Updated in b033218.