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
Cleaning up of snapshots and events before snapshots based on time and a # of snapshots #828
Conversation
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.
LGTM, some smaller nitpicky stuff mostly
core/src/main/scala/akka/persistence/cassandra/snapshot/CassandraSnapshotStore.scala
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/cassandra/snapshot/CassandraSnapshotStore.scala
Outdated
Show resolved
Hide resolved
def deleteBeforeSnapshot( | ||
persistenceId: String, | ||
snapshotsToKeep: Int, | ||
keepAfter: Long): Future[Option[SnapshotMetadata]] = { |
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.
Hard to parse what it does from name/docs but I'm not sure I can come up with anything better...
deleteSnapshotsBefore(persistenceId, unixTimestamp, nrSnapshotsToKeep)
?
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.
Yah i struggled to describe it.
Just having a timestamp makes it easy to understand "delete all snapshots older than X" but is an API that will lead to ppl deleting all their snapshots!
|
||
/** | ||
* Deletes all but the last N snapshots and deletes all events before this snapshot | ||
* Does not delete from the tag_views table |
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.
Perhaps additional caveat about consequences for this? Events by tag would fail if you run it and see a tag for a message that is not there?
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.
I don't think it'll fail, it doesn't reconcile that way around. Only messages to tag_views table. I will add warnings tho
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.
looking good, a few questions
core/src/main/scala/akka/persistence/cassandra/cleanup/Cleanup.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/cassandra/journal/CassandraJournal.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/cassandra/snapshot/CassandraSnapshotStatements.scala
Show resolved
Hide resolved
val allRows: Source[Row, NotUsed] = session.select(ps.bind(persistenceId)) | ||
@volatile var count = 0 | ||
allRows | ||
.takeWhile { row => |
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.
could we skip the count hack with:
allRows
.take(snapshotsToKeep)
.takeWhile { row =>
?
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.
No as we may wan to keep more than snapshotsToKeep if they are all newer than the toKeep time
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.
LGTM, after some small doc additions
ae39644
to
f080bd1
Compare
Cleanup snapshots and events from before a configured snapshot (#828) (for 0.x branch)
No description provided.