The daily aggregates go when their own retention window says so - #322
Merged
Merged
Conversation
DailyAggregateRetentionDays was on the settings page, was validated and was stored, and no run read it. A daily aggregate was kept for ever whatever the number said, while the raw rows behind it went at ninety days by default. That is the opposite of the sentence the two settings carry about the two windows, and it is the state an operator plans a retention around. What changed: - IPlayStore gains CountRollupsBefore and DeleteRollupsBefore, over days rather than over moments, because a rollup is keyed to a calendar day. The deletion carries a bite for the reason the play deletion does: one statement removing a decade of rollups holds the write lock for its whole duration and answers no cancellation while it runs. - RetentionSweep takes a second cutoff and answers both windows in one run. A second scheduled task on the same schedule and the same store is how two windows drift apart until one is quietly not running, so this is one sweep with two cutoffs. - The day the aggregate window ends on is read in the zone the store states its rollups were counted in, not the machine's. A cutoff converted through a runner's zone is a deletion boundary that moves by a day depending on where the server is. A store that has keyed no rollup states no zone and is not asked for a day. - The aggregates go before the play rows, and that order is the property rather than an arrangement of two loops. A run stopped between the two leaves the rows the deleted aggregates were folded from in the file, so what went can be folded again; the other order takes those rows away first and makes the same deletion terminal on a server whose settings say it is not. - The deletion is recorded as Retention, and DeletionClass says why on the member a reader of that vocabulary meets. An aggregate deleted because it is older than its window went away for the same reason a play row does, and recording it as Corrective would give one reason two names depending on which table it happened in. That deleting a rollup can destroy the only remaining record of a period is a consequence rather than a second reason: it follows from which of the two windows is larger, so the same removal would carry different classes on two servers if it were written on the row. It is said beside the settings instead. - Run returns what it removed per table. A caller handed the sum could not tell a server that deleted a year of sessions from one that deleted a year of summaries. A CASE THAT CLAIMED A WINDOW WAS READ AT THE RUN DID NOT PROVE IT, and this repairs that as well because the new case would have copied the defect. Both cases moved a field on the configuration object they had already handed the task, so a task that read its configuration once and held it saw the mutation too and stayed green. Measured: holding the first reading in a field left the play-row case passing. Each case now hands a different object on the second run, and both go red under that near miss. Every guard here was proven by breaking it and watching the suite go red: the boundary comparison widened to include the first day kept, the bite dropped from the delete, the class swapped to Corrective, the zone guard replaced by a fallback to UTC, the configuration held from the first run, and the two loops put in the other order. docs/configuration.md stops recording this setting as read by nothing and says which of the two numbers being larger makes the aggregate deletion terminal rather than recoverable, because nothing on the page tells an operator which they have chosen. docs/plugin-data.md says what the sweep deletes and when. The size is argued rather than left to the reader: one property holds across the change, which is that a second window exists and is answered by the sweep that already exists, and fourteen of the twenty-six files are the store interface arriving at the fakes that implement it. Closes #315. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #315.
DailyAggregateRetentionDayswas on the settings page, was validated and was stored, and no run read it. A daily aggregate was kept for ever whatever the number said, while the raw rows behind it went at ninety days by default — the opposite of the sentence the two settings carry about the two windows.The means
C#, in the plugin assembly and its suite, because the artefact is a scheduled task inside a Jellyfin plugin and the store, the sweep and the vocabulary it extends are already there. Nothing here adds a language, a runtime or a dependency the tree does not carry, the properties are refusable by the suite that already runs, and every claim below carries the command that produced it. A second means would have meant a second apparatus for one deletion.
What is in it
IPlayStoregainsCountRollupsBeforeandDeleteRollupsBefore, over days rather than moments, because a rollup is keyed to a calendar day. The deletion carries a bite for the reason the play deletion does.RetentionSweeptakes a second cutoff and answers both windows in one run. A second scheduled task on the same schedule and the same store is how two windows drift apart until one is quietly not running.Retention, andDeletionClasssays why on the member a reader of that vocabulary meets.Runreturns what it removed per table rather than one sum.Every guard was proven by breaking it
Each near miss below was applied to the working tree on its own, the suite run, and the edit reverted.
dotnet test Jellyfin.Plugin.Stats.sln -f net9.0 -p:CollectCoverage=false, 1048 cases in the baseline:WHERE Day < $daywidened to<=LIMIT $limitdropped from the deleteDeletionClass.Retentionswapped toCorrectiveTimeZoneInfo.UtcA case that claimed a window was read at the run did not prove it
This is a defect in what was already merged, and it is repaired here because the new case would otherwise have copied it.
TheWindowIsReadAtTheRunAndNotHeldFromBeforemoved a field on the configuration object it had already handed the task, so a task that read its configuration once and held it saw the mutation too and stayed green. Measured: the fifth near miss above left that case passing until it was rewritten. Both cases now hand a different object on the second run.The suite and the gate
The branch threshold in
coverage.propsis 100 and is met on both frameworks; the new branches are covered by the boundary case, the null-zone case and the cancellation case rather than by a threshold moving.One invariant did fire during the work and the code changed rather than the rule:
no-configuration-value-in-a-static-fieldread aprivate static PluginConfiguration AConfiguration(...)test helper as a static holding a setting. The helper is gone and each case writes its own object.Size
850 insertions over 26 files, against the 400-line guide, and it is one readable thing rather than two changes. One property holds across it — a second window exists and is answered by the sweep that already exists — and 14 of the 26 files are the two new interface members arriving at the fakes that implement
IPlayStore. Re-planning was considered and rejected: an interface member and its only implementation cannot land apart without an unimplemented interface in between, and the sweep, the class decision and the two documents are the same statement said in four places.What this does not do
docs/configuration.mdnow says so where the number is typed.Scope
The issue declares
Jellyfin.Plugin.Stats/ScheduledTasks/,Jellyfin.Plugin.Stats/Data/,docs/configuration.mdanddocs/plugin-data.md. The change also touchesJellyfin.Plugin.Stats.Tests/, which the issue's own done-conditions ask for and which itsScope:line does not name.