feat: batch expired snapshots

[eakmanrq]

Batches the deletion of expired snapshots by paging over the candidates using the updated_at, name, and identifier as stable fields to do the paging. Simple viz explaining what is going on (just shows updated_at but know that it includes all 3 columns):

In that example, 3 batches are processed. Getting expired snapshots a limited since it would be unbounded otherwise. Instead of passing though snapshots directly into delete instead the "boundary" is passed into the delete. That becomes the "upper boundary" for the delete to then look backwards to perform the deletes. Limit is not needed since this is bounded. There is an underlying assumption that these upper boundaries are provided in paged order (on other words, the last batch isn't provided first since that would remove the benefit of batching).

This approach compared to passing in the deleted snapshot ids directly has the following benefits:

• In a client/server architecture this places less trust on the input from the client by asking for just the boundary instead of the explicit list of snapshot ids to delete
• Also in client/server architecture, less information needs to be passed to perform the delete
• If something were to change between the calculation of the batch and the eventual deletion then this approach will correctly not delete the snapshot while if the explicit list if provided then the delete would still happen

[erindru]

I don't quite grok UpperBatchBoundary vs LowerBatchBoundary and when to use one vs the other. I can see the problem that it solves though

[erindru]

Was there a reason to make this t.Optional vs just returning an empty list if there are no expired snapshots like it did before?

[eakmanrq]

Yes that is correct

[izeigerman]

IMHO, I don't think this belongs here. I don't believe State Sync should have anything to do with SnapshotEvaluator. Plus this creates a circular dependency between modules.

The logical relationship should be SnapshotEvaluator -> StateSync.

Anticipating your question - yes, I think putting cleanup_expired_views in here was a mistake.

[eakmanrq]

Where do you want it located then?

[izeigerman]

perhaps it's time to create the core/janitor.py module?

[eakmanrq]

Can we consider this out of scope for this PR? I'm open to following up if you think it is important but it doesn't seem like this should be required to get this change in.

[izeigerman]

sure. But I do think it's important