Conversation
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.
two follow-ups from the #34 review: the weighted-mode config validation, plus the apply_batch transaction fix you agreed to fold in here.
config validation
the
weightedpriority-mode weights andwithout-index-compensation-factorare formatted directly into the score SQL (store/prioritization.rs) with nothing validating them. two ways a bad value bites at runtime:inf/nanfrom TOML) renders as a bareinf/NaNtoken that Postgres reads as an unknown column, so every create/evict/regression pass errors out and no index gets created or evicted.(1 + w*...)volume factor and inverts the ranking (highest-demand pattern sorts as least valuable).validated at load instead: the four weights must be finite and
>= 0, the compensation factor finite and> 0. two smalldeserialize_withhelpers, the same pattern as the existingdeserialize_duration_requiredon the neighbouringrate-windowfield. tests cover a valid config loading plus rejection of a negative weight,inf,nan, and a non-positive compensation factor, through the realfrom_strpath.apply_batch atomicity
apply_batchcommitted each observation in its own transaction, so a mid-batch database error left the earlier ones committed while the client re-buffered and retried the whole chunk, re-applying that prefix and inflating demand/cost. it's silent: the double-apply lands on the retry, and nothing logs at the point the counters inflate.record_demandnow runs against a caller-provided connection andapply_batchwraps the whole batch in one transaction, so a failure rolls everything back and the retry re-applies cleanly, which is what the client's re-buffer already assumes.one thing that falls out of using a single transaction: it now holds every pattern's row lock until commit, where the per-observation transactions held one at a time. so observations are sorted by
pattern_idbefore the upserts, giving two concurrent batches that share patterns a consistent lock order so they can't deadlock.defaults are unchanged. fmt and clippy clean. the config validation has unit tests; the apply_batch change is compile and clippy verified, exercising the ingest path end to end needs a live Postgres.