Skip to content
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.

Sign up for GitHub

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

fix: condition for deleting old timestamp offsets #1186

Merged
merged 3 commits into from
Sep 3, 2024
+174 −5
Merged

fix: condition for deleting old timestamp offsets #1186

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ccda69
fix: condition for deleting old timestamp offsets
pvlugter Aug 28, 2024
5e4b32c
add test cases for eviction/deletion overlap scenarios
pvlugter Aug 29, 2024
3673969
always trigger deletion after eviction
pvlugter Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion akka-projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/R2dbcOffsetStore.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,7 +723,7 @@ private[projection] class R2dbcOffsetStore(
Future.successful(0)
} else {
val currentState = getState()
if (currentState.size <= settings.keepNumberOfEntries || currentState.window.compareTo(settings.timeWindow) < 0) {
if (currentState.size <= settings.keepNumberOfEntries && currentState.window.compareTo(settings.timeWindow) < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think there could be something else wrong here.

  • evict is about what we remove from memory, state holds one entry per pid
  • delete is about deleting old rows from the database, one row per event (some batching can occur)

With few pids, less than keepNumberOfEntries, we would never delete from db.

Also, when > keepNumberOfEntries we evict, and thereby state.oldestTimestamp is updated to the oldest we have in memory, but there could still be older in db. Meaning that if we evict all the time we might never delete. Now we also have the evictThresholdReached so we don't evict all the time and thereby it might anyway delete.

However, feels like we should have two separate oldestTimestamp, one for what is in memory, and another for what is in the db.

Then this delete condition should maybe not involve currentState.size, but only look at the db time window. Alternatively, have a separate counter for inserted records and let that drive when a delete should be performed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With few pids, less than keepNumberOfEntries, we would never delete from db.

Yes, that's what was being seen, and the only thing this PR is fixing. It now triggers deletion based on either the number of entries or the window (where this is a negative condition so needs to be and/&&).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was going to mention possible issues around eviction overlapping with this. The eviction is:

if (oldestTimestamp.isBefore(until) && size > keepNumberOfEntries) {
  val newState = State(
    sortedByTimestamp.take(size - keepNumberOfEntries).filterNot(_.timestamp.isBefore(until))
    ++ sortedByTimestamp.takeRight(keepNumberOfEntries)
    ++ latestBySlice)

So it only evicts when there's both the oldest timestamp is too old and the number of entries have gone over the limit. In the example case that prompted this PR, the number of entries is always below the limit, so there won't be any eviction. Allowing the deletion trigger to just be based on the time window.

For the case where there are more persistence ids, it could certainly trigger eviction in a way that prevents deletion — but may trigger on subsequent timer. The latest by slice are always kept, so would trigger if the time range across slices is bigger than the time window. Or if new persistence ids are being seen, so that it evicts (reducing to the limit), then new persistence ids (but no older timestamps, so doesn't evict yet), then deletion timer and triggers deletion based on size.

Agree that the overlaps between eviction and deletion need to be addressed in some way. Could be clearer with some separate mechanism.

Or a simple fix for this could be to have eviction trigger deletion. So if it has evicted entries, set a flag for the next deletion timer to always trigger. Similar to the idle flag that's already there to prevent unnecessary deletion. And then remove the condition based on size, which will already be there based on eviction.

I'll look at creating some test scenarios for the different cases.

Copy link
Contributor Author

@pvlugter pvlugter Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests for these scenarios, in 5e4b32c

  • deleting based on time window when fewer pids than limit (for the first fix in this PR)
  • still deleting after eviction because there's an old latest by slice that was retained
  • deleting after eviction because new persistence ids arrive (while still within time window)
  • the previous scenario also tests the issue of not triggering deletion because oldest updated from eviction

Then updated from having the condition on keep-number-of-entries to always triggering next deletion after an eviction (which has checked for both over limit and outside time window). Still triggers deletion on time window, for when fewer pids than limit (no eviction). Last test scenarios updated for the new trigger. See 9e0e35a 3673969.

// it hasn't filled up the window yet
Future.successful(0)
} else {
Expand Down
Loading