fix: condition for deleting old timestamp offsets

[pvlugter]

To trigger deletion of old timestamp offsets, the current condition is that both the number of entries (persistence ids) is over the limit (10000 by default) and the oldest to latest offset window is longer than the time window. Wonder if that was intended to instead be if it's either over the limit or outside the window to trigger deletion?

[patriknw]

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.

[pvlugter]

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/&&).

[pvlugter]

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.

[pvlugter]

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.

[patriknw]

LGTM