Delete expired known endpoints #2108
Conversation
|
|
||
| try | ||
| { | ||
| var query = new IndexQuery |
There was a problem hiding this comment.
I first wanted to use something like
Operation operation = store
.DatabaseCommands
.DeleteByIndex(
"Raven/DocumentsByEntityName",
new IndexQuery
{
Query = "Tag:KnownEndpoints AND LastSeen:[* TO {expiryThreshold.Ticks}]"
},
new BulkOperationOptions
{
AllowStale = false
});
but then I wasn't able to find delete by index on the document database structure and I realized in order to filter by LastSeen we require an index anyway
There was a problem hiding this comment.
Elsewhere in the app, we're making the assumption that the total number of known endpoints will generally be contained to < 1024. So why are we doing the select + delete ids in batch dance when we could do a DeleteByIndex using ExpiryKnownEndpointsIndex?
There was a problem hiding this comment.
Cleaning runs in a bundle, there I have only access to the Document Database type. Like written before I couldn't find a delete by index so I used the same approach as in the audit cleaner to use the batch size settings to chunk for convenience. Regarding the settings yeah we kind of hardcoded page size to be 1024 for now and cutt off the query but we also introduced paging in the other PR so I'm on the fence.
Do you know of a way of doing delete by query on the Document Database object by any chance?
There was a problem hiding this comment.
Ah, I forgot this was in a bundle. 🤦
There was a problem hiding this comment.
Have we considered just setting the Raven Expiration Date, sliding it forward for each new entry, and relying on the built-in retention bundle to do the job?
https://ravendb.net/docs/article-page/3.5/Csharp/server/bundles/expiration
There was a problem hiding this comment.
I dismissed Expiration for now because as of today Expiration is a business feature. Meaning if someone changes the expiration time it will affect all running cycles after the date has been set. This is also doable with Expiration but we would have to apply a set based patch then on the endpoints. I think this is likely coming up again as a discussion point when upgrading Raven so I figured I just stick to the current pattern
| logger.Debug("Trying to find expired KnownEndpoints to delete (with threshold {0})", threshold.ToString(Default.DateTimeFormatsToWrite, CultureInfo.InvariantCulture)); | ||
| KnownEndpointsCleaner.Clean(deletionBatchSize, database, threshold, token); |
There was a problem hiding this comment.
I wonder if it's wise to execute this every time, as in most systems this data (while updating the LastSeen parameter) should remain relatively static. Maybe it should have a separate schedule, or run every 10th time?
There was a problem hiding this comment.
I saw no reason to diverge from the cleaner pattern we also use in the primary instance
Right now it is assumed that the expiry interval and settings apply to all cleanups. Given that it is an async process that runs on a customer defined schedule I saw no problem and it keeps the data consistent with the expiry of processed messages so essentially mimicing more or less the current behaviour.
There was a problem hiding this comment.
The collection is also likely to be relatively small so the overhead of checking it will be negligible compared to the audit cleanup
|
|
||
| try | ||
| { | ||
| var query = new IndexQuery |
There was a problem hiding this comment.
Elsewhere in the app, we're making the assumption that the total number of known endpoints will generally be contained to < 1024. So why are we doing the select + delete ids in batch dance when we could do a DeleteByIndex using ExpiryKnownEndpointsIndex?
| @@ -16,6 +16,8 @@ public static Task<TimerJobExecutionResult> RunCleanup(int deletionBatchSize, Do | |||
|
|
|||
| logger.Debug("Trying to find expired ProcessedMessage and SagaHistory documents to delete (with threshold {0})", threshold.ToString(Default.DateTimeFormatsToWrite, CultureInfo.InvariantCulture)); | |||
| AuditMessageCleaner.Clean(deletionBatchSize, database, threshold, token); | |||
| logger.Debug("Trying to find expired KnownEndpoints to delete (with threshold {0})", threshold.ToString(Default.DateTimeFormatsToWrite, CultureInfo.InvariantCulture)); | |||
There was a problem hiding this comment.
Why have a separate log line for this and not just include it in the one above? The threshold is the same
There was a problem hiding this comment.
I stuck to the pattern started in
but even there it is kind of not the same because ProcessedMessages and SagaHistory are in one log message. Happy to change
| logger.Debug("Trying to find expired KnownEndpoints to delete (with threshold {0})", threshold.ToString(Default.DateTimeFormatsToWrite, CultureInfo.InvariantCulture)); | ||
| KnownEndpointsCleaner.Clean(deletionBatchSize, database, threshold, token); |
There was a problem hiding this comment.
The collection is also likely to be relatively small so the overhead of checking it will be negligible compared to the audit cleanup
|
|
||
| try | ||
| { | ||
| var query = new IndexQuery |
There was a problem hiding this comment.
Have we considered just setting the Raven Expiration Date, sliding it forward for each new entry, and relying on the built-in retention bundle to do the job?
https://ravendb.net/docs/article-page/3.5/Csharp/server/bundles/expiration
|
Merging for now. Will consolidate the logging in the other PR |
No description provided.