fix(inkless:retention): decouple enforce_retention_v2 from the log lock - #705
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Restructures Postgres retention enforcement (enforce_retention_v2) to reduce commit tail-latency by avoiding holding the logs row lock during the O(depth) boundary scan, and updates the Java job to avoid lock accumulation across partitions by running one transaction per request.
Changes:
- Updates
enforce_retention_v2to compute the retention boundary withoutFOR UPDATE, then acquire the lock only for the bounded delete/recount section. - Changes
EnforceRetentionJobto execute one DB transaction per retention request (sharednowper cycle). - Adds Postgres concurrency regression tests plus an opt-in benchmark to measure commit blocking during enforcement.
Reviewed changes
Copilot reviewed 4 out of 122 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| storage/inkless/src/main/java/io/aiven/inkless/control_plane/postgres/EnforceRetentionJob.java | Runs retention enforcement in per-request transactions and expects a single response per call. |
| storage/inkless/src/main/resources/db/migration/V21__Retention_enforcement_lock_decoupling.sql | Rewrites enforce_retention_v2 to lock late (delete phase only) and adds short-circuit + capped-delete logic. |
| storage/inkless/src/test/java/io/aiven/inkless/control_plane/postgres/EnforceRetentionJobTest.java | Adds concurrency-focused tests for scan/delete interleavings and NULL earliest_batch_timestamp behavior. |
| storage/inkless/src/test/java/io/aiven/inkless/control_plane/postgres/EnforceRetentionCommitBlockingBenchmarkTest.java | Adds an opt-in @Tag("benchmark") harness to measure commit blocking during enforcement. |
| storage/inkless/src/main/jooq/org/jooq/generated/DefaultSchema.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/Domains.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/Indexes.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/Keys.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/Routines.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/Tables.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/UDTs.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/AdvanceCrossTierLogStartResponseErrorV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/CommitBatchResponseErrorV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/DeleteRecordsResponseErrorV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/EnforceRetentionResponseErrorV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/FileReasonT.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/FileStateT.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/FindBatchesResponseErrorV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/InitDisklessLogErrorV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/ListOffsetsResponseErrorV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/enums/PruneBatchesBelowHighestTieredOffsetErrorV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/routines/BatchTimestamp.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/routines/DeleteBatchV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/routines/DeleteFilesV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/routines/DeleteTopicV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/routines/FlushCommitRunV2.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/routines/MarkFileToDeleteV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/AdvanceCrossTierLogStartV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/Batches.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/CommitFileV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/CommitFileV2.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/DeleteRecordsV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/EnforceRetentionV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/EnforceRetentionV2.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/Files.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/FindBatchesV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/FindBatchesV2.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/InitDisklessLogV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/ListOffsetsV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/Logs.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/ProducerState.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/PruneBatchesBelowHighestTieredOffsetV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/RepairDisklessLogV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/AdvanceCrossTierLogStartV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/BatchesRecord.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/CommitFileV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/CommitFileV2Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/DeleteRecordsV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/EnforceRetentionV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/EnforceRetentionV2Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/FilesRecord.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/FindBatchesV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/FindBatchesV2Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/InitDisklessLogV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/ListOffsetsV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/LogsRecord.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/ProducerStateRecord.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/PruneBatchesBelowHighestTieredOffsetV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/tables/records/RepairDisklessLogV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/AdvanceCrossTierLogStartRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/AdvanceCrossTierLogStartResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/BatchInfoV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/BatchMetadataV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/CommitBatchRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/CommitBatchResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/DeleteRecordsRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/DeleteRecordsResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/EnforceRetentionRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/EnforceRetentionResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/FindBatchesRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/FindBatchesResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/InitDisklessLogProducerStateV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/InitDisklessLogRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/InitDisklessLogResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/ListOffsetsRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/ListOffsetsResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/PruneBatchesBelowHighestTieredOffsetRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/PruneBatchesBelowHighestTieredOffsetResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/RepairDisklessLogRequestV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/RepairDisklessLogResponseV1.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/AdvanceCrossTierLogStartRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/AdvanceCrossTierLogStartResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/BatchInfoV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/BatchMetadataV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/CommitBatchRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/CommitBatchResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/DeleteRecordsRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/DeleteRecordsResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/EnforceRetentionRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/EnforceRetentionResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/FindBatchesRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/FindBatchesResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/InitDisklessLogProducerStateV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/InitDisklessLogRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/InitDisklessLogResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/ListOffsetsRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/ListOffsetsResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/PruneBatchesBelowHighestTieredOffsetRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/PruneBatchesBelowHighestTieredOffsetResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/RepairDisklessLogRequestV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/RepairDisklessLogResponseV1Path.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/AdvanceCrossTierLogStartRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/AdvanceCrossTierLogStartResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/BatchInfoV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/BatchMetadataV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/CommitBatchRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/CommitBatchResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/DeleteRecordsRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/DeleteRecordsResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/EnforceRetentionRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/EnforceRetentionResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/FindBatchesRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/FindBatchesResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/InitDisklessLogProducerStateV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/InitDisklessLogRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/InitDisklessLogResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/ListOffsetsRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/ListOffsetsResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/PruneBatchesBelowHighestTieredOffsetRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/PruneBatchesBelowHighestTieredOffsetResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/RepairDisklessLogRequestV1Record.java | jOOQ regen metadata bump to schema version 21. |
| storage/inkless/src/main/jooq/org/jooq/generated/udt/records/RepairDisklessLogResponseV1Record.java | jOOQ regen metadata bump to schema version 21. |
Measures commit latency while enforce_retention_v2 runs on the same partition, as an A/B harness for the lock-decoupling change. Two shapes: single-partition (commit and enforce target the same partition, isolating the scan-under-lock effect) and multi-partition (enforce runs over N partitions while a commit targets the first, isolating lock accumulation across partitions in one transaction). Tagged @tag("benchmark") so it is excluded from CI and run on demand via the benchmarkTest task.
jeqo
force-pushed
the
jeqo/fix-enforcer
branch
2 times, most recently
from
July 17, 2026 10:04
4f5ffbf to
55f56bc
Compare
enforce_retention_v2 took logs FOR UPDATE before its O(depth) retention boundary scan and looped every request in one transaction, so a concurrent commit_file on a locked partition waited for the whole enforcement pass. In production the commit-latency tail tracked the enforce latency (commit max ~ enforce max). Restructure enforcement so the lock is held only for the short delete: - The retention boundary is now computed in a read-only step with no FOR UPDATE. byte_size is read inside that scan statement (a selected_log CTE) so the reverse-aggregate stays consistent with the batches rows under READ COMMITTED. The lock is taken only afterwards, for the delete. - EnforceRetentionJob runs one transaction per request instead of wrapping all requests in one. A plpgsql function cannot release a row lock before it returns, so per-request transactions are what actually free the lock between partitions rather than accumulating it until the end. now is computed once per cycle and shared. Each transaction holds at most one partition lock, so enforce can no longer self-deadlock and the loop's ordering is no longer needed for deadlock avoidance on this path. - Deleted batches/bytes are recounted under the lock so the reported counts stay accurate when a concurrent delete advances log_start between the unlocked scan and the locked delete. - The max_batches_per_request cap now probes for the (cap+1)-th deletable batch with LIMIT/OFFSET instead of COUNT(*) over all deletable rows, keeping the locked section O(delete-size) rather than O(depth) when a small cap meets a large deletable set. - The boundary scan is skipped entirely when the oldest retained batch provably survives every enabled policy, using logs.byte_size and logs.earliest_batch_timestamp. A NULL earliest_batch_timestamp means "unknown", so the short-circuit does not fire and the scan runs. Computing the boundary unlocked is safe: retention deletes at the tail while commit appends at the head, and delete_records_v1 re-clamps to the current log_start under its own lock, so a concurrent commit or delete cannot cause over-deletion. Partial progress across partitions is fine; retention is idempotent and re-runs next cycle. Adds PG concurrency tests for the interleavings this enables: log deleted between scan and delete returns unknown_topic_or_partition; log_start advanced between scan and delete returns the recounted deletion; and a NULL earliest_batch_timestamp falls back to the scan instead of short-circuiting. Regenerates jOOQ sources for the schema-version stamp bump introduced by the migration.
… control plane Mirror logs.earliest_batch_timestamp into InMemoryControlPlane for parity with the Postgres control plane, so the enforce_retention short-circuit behaves identically across both backends. - LogInfo.earliestBatchTimestamp (nullable; null == "unknown, must scan", matching the SQL NULL sentinel), maintained under the same rules as V20: commit populates only when unset, delete/prune/enforce recompute from the new oldest batch when the oldest changed (null when the log empties). - enforceRetention gains the same short-circuit: when the oldest retained batch survives every enabled policy, skip the O(depth) selection scans. It skips only the scans and falls through to the unchanged tail, so the reported log_start_offset and deletion counts are identical (pure no-op result-wise; the in-memory backend has no O(depth) scan to optimize, this is parity only). - InMemoryControlPlaneEarliestBatchTimestampTest mirrors the Postgres LogsEarliestBatchTimestampTest across commit/delete/prune/enforce. The field is intentionally not on the ControlPlane interface, so it is read via reflection.
…art backward InMemoryControlPlane.enforceRetention recomputed log_start_offset from the oldest batch's base offset on every call, even when it deleted nothing. If a prior deleteRecords had advanced log_start into the middle of a still-retained batch (e.g. offset 5 inside batch [0,9]), a later enforce that deleted nothing would reset log_start back to that batch's base (0). That diverges from delete_records_v1 (Postgres) and re-exposes reads for offsets that should be below the log start. Only touch log_start (and the mirrored earliest_batch_timestamp) when a deletion actually occurred, and make the recompute forward-only. Adds a parity test in AbstractControlPlaneTest.Retention that runs against both backends: after deleteRecords advances log_start mid-batch, an enforce that deletes nothing must leave log_start unchanged.
jeqo
marked this pull request as ready for review
July 17, 2026 11:50
jeqo
added a commit
that referenced
this pull request
Jul 20, 2026
…ck (#705) * test(inkless:retention): add enforce commit-blocking benchmark Measures commit latency while enforce_retention_v2 runs on the same partition, as an A/B harness for the lock-decoupling change. Two shapes: single-partition (commit and enforce target the same partition, isolating the scan-under-lock effect) and multi-partition (enforce runs over N partitions while a commit targets the first, isolating lock accumulation across partitions in one transaction). Tagged @tag("benchmark") so it is excluded from CI and run on demand via the benchmarkTest task. * fix(inkless:retention): decouple enforce_retention_v2 from the log lock enforce_retention_v2 took logs FOR UPDATE before its O(depth) retention boundary scan and looped every request in one transaction, so a concurrent commit_file on a locked partition waited for the whole enforcement pass. In production the commit-latency tail tracked the enforce latency (commit max ~ enforce max). Restructure enforcement so the lock is held only for the short delete: - The retention boundary is now computed in a read-only step with no FOR UPDATE. byte_size is read inside that scan statement (a selected_log CTE) so the reverse-aggregate stays consistent with the batches rows under READ COMMITTED. The lock is taken only afterwards, for the delete. - EnforceRetentionJob runs one transaction per request instead of wrapping all requests in one. A plpgsql function cannot release a row lock before it returns, so per-request transactions are what actually free the lock between partitions rather than accumulating it until the end. now is computed once per cycle and shared. Each transaction holds at most one partition lock, so enforce can no longer self-deadlock and the loop's ordering is no longer needed for deadlock avoidance on this path. - Deleted batches/bytes are recounted under the lock so the reported counts stay accurate when a concurrent delete advances log_start between the unlocked scan and the locked delete. - The max_batches_per_request cap now probes for the (cap+1)-th deletable batch with LIMIT/OFFSET instead of COUNT(*) over all deletable rows, keeping the locked section O(delete-size) rather than O(depth) when a small cap meets a large deletable set. - The boundary scan is skipped entirely when the oldest retained batch provably survives every enabled policy, using logs.byte_size and logs.earliest_batch_timestamp. A NULL earliest_batch_timestamp means "unknown", so the short-circuit does not fire and the scan runs. Computing the boundary unlocked is safe: retention deletes at the tail while commit appends at the head, and delete_records_v1 re-clamps to the current log_start under its own lock, so a concurrent commit or delete cannot cause over-deletion. Partial progress across partitions is fine; retention is idempotent and re-runs next cycle. Adds PG concurrency tests for the interleavings this enables: log deleted between scan and delete returns unknown_topic_or_partition; log_start advanced between scan and delete returns the recounted deletion; and a NULL earliest_batch_timestamp falls back to the scan instead of short-circuiting. Regenerates jOOQ sources for the schema-version stamp bump introduced by the migration. * feat(inkless:retention): mirror earliest_batch_timestamp to in-memory control plane Mirror logs.earliest_batch_timestamp into InMemoryControlPlane for parity with the Postgres control plane, so the enforce_retention short-circuit behaves identically across both backends. - LogInfo.earliestBatchTimestamp (nullable; null == "unknown, must scan", matching the SQL NULL sentinel), maintained under the same rules as V20: commit populates only when unset, delete/prune/enforce recompute from the new oldest batch when the oldest changed (null when the log empties). - enforceRetention gains the same short-circuit: when the oldest retained batch survives every enabled policy, skip the O(depth) selection scans. It skips only the scans and falls through to the unchanged tail, so the reported log_start_offset and deletion counts are identical (pure no-op result-wise; the in-memory backend has no O(depth) scan to optimize, this is parity only). - InMemoryControlPlaneEarliestBatchTimestampTest mirrors the Postgres LogsEarliestBatchTimestampTest across commit/delete/prune/enforce. The field is intentionally not on the ControlPlane interface, so it is read via reflection. * fix(inkless:retention): stop in-memory enforceRetention moving log_start backward InMemoryControlPlane.enforceRetention recomputed log_start_offset from the oldest batch's base offset on every call, even when it deleted nothing. If a prior deleteRecords had advanced log_start into the middle of a still-retained batch (e.g. offset 5 inside batch [0,9]), a later enforce that deleted nothing would reset log_start back to that batch's base (0). That diverges from delete_records_v1 (Postgres) and re-exposes reads for offsets that should be below the log start. Only touch log_start (and the mirrored earliest_batch_timestamp) when a deletion actually occurred, and make the recompute forward-only. Adds a parity test in AbstractControlPlaneTest.Retention that runs against both backends: after deleteRecords advances log_start mid-batch, an enforce that deletes nothing must leave log_start unchanged.
jeqo
added a commit
that referenced
this pull request
Jul 20, 2026
…ck (#705) * test(inkless:retention): add enforce commit-blocking benchmark Measures commit latency while enforce_retention_v2 runs on the same partition, as an A/B harness for the lock-decoupling change. Two shapes: single-partition (commit and enforce target the same partition, isolating the scan-under-lock effect) and multi-partition (enforce runs over N partitions while a commit targets the first, isolating lock accumulation across partitions in one transaction). Tagged @tag("benchmark") so it is excluded from CI and run on demand via the benchmarkTest task. * fix(inkless:retention): decouple enforce_retention_v2 from the log lock enforce_retention_v2 took logs FOR UPDATE before its O(depth) retention boundary scan and looped every request in one transaction, so a concurrent commit_file on a locked partition waited for the whole enforcement pass. In production the commit-latency tail tracked the enforce latency (commit max ~ enforce max). Restructure enforcement so the lock is held only for the short delete: - The retention boundary is now computed in a read-only step with no FOR UPDATE. byte_size is read inside that scan statement (a selected_log CTE) so the reverse-aggregate stays consistent with the batches rows under READ COMMITTED. The lock is taken only afterwards, for the delete. - EnforceRetentionJob runs one transaction per request instead of wrapping all requests in one. A plpgsql function cannot release a row lock before it returns, so per-request transactions are what actually free the lock between partitions rather than accumulating it until the end. now is computed once per cycle and shared. Each transaction holds at most one partition lock, so enforce can no longer self-deadlock and the loop's ordering is no longer needed for deadlock avoidance on this path. - Deleted batches/bytes are recounted under the lock so the reported counts stay accurate when a concurrent delete advances log_start between the unlocked scan and the locked delete. - The max_batches_per_request cap now probes for the (cap+1)-th deletable batch with LIMIT/OFFSET instead of COUNT(*) over all deletable rows, keeping the locked section O(delete-size) rather than O(depth) when a small cap meets a large deletable set. - The boundary scan is skipped entirely when the oldest retained batch provably survives every enabled policy, using logs.byte_size and logs.earliest_batch_timestamp. A NULL earliest_batch_timestamp means "unknown", so the short-circuit does not fire and the scan runs. Computing the boundary unlocked is safe: retention deletes at the tail while commit appends at the head, and delete_records_v1 re-clamps to the current log_start under its own lock, so a concurrent commit or delete cannot cause over-deletion. Partial progress across partitions is fine; retention is idempotent and re-runs next cycle. Adds PG concurrency tests for the interleavings this enables: log deleted between scan and delete returns unknown_topic_or_partition; log_start advanced between scan and delete returns the recounted deletion; and a NULL earliest_batch_timestamp falls back to the scan instead of short-circuiting. Regenerates jOOQ sources for the schema-version stamp bump introduced by the migration. * feat(inkless:retention): mirror earliest_batch_timestamp to in-memory control plane Mirror logs.earliest_batch_timestamp into InMemoryControlPlane for parity with the Postgres control plane, so the enforce_retention short-circuit behaves identically across both backends. - LogInfo.earliestBatchTimestamp (nullable; null == "unknown, must scan", matching the SQL NULL sentinel), maintained under the same rules as V20: commit populates only when unset, delete/prune/enforce recompute from the new oldest batch when the oldest changed (null when the log empties). - enforceRetention gains the same short-circuit: when the oldest retained batch survives every enabled policy, skip the O(depth) selection scans. It skips only the scans and falls through to the unchanged tail, so the reported log_start_offset and deletion counts are identical (pure no-op result-wise; the in-memory backend has no O(depth) scan to optimize, this is parity only). - InMemoryControlPlaneEarliestBatchTimestampTest mirrors the Postgres LogsEarliestBatchTimestampTest across commit/delete/prune/enforce. The field is intentionally not on the ControlPlane interface, so it is read via reflection. * fix(inkless:retention): stop in-memory enforceRetention moving log_start backward InMemoryControlPlane.enforceRetention recomputed log_start_offset from the oldest batch's base offset on every call, even when it deleted nothing. If a prior deleteRecords had advanced log_start into the middle of a still-retained batch (e.g. offset 5 inside batch [0,9]), a later enforce that deleted nothing would reset log_start back to that batch's base (0). That diverges from delete_records_v1 (Postgres) and re-exposes reads for offsets that should be below the log start. Only touch log_start (and the mirrored earliest_batch_timestamp) when a deletion actually occurred, and make the recompute forward-only. Adds a parity test in AbstractControlPlaneTest.Retention that runs against both backends: after deleteRecords advances log_start mid-batch, an enforce that deletes nothing must leave log_start unchanged.
jeqo
added a commit
that referenced
this pull request
Jul 27, 2026
…O(cap) The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every deletable batch, O(partition depth). PR #705 moved it out of the log lock so it no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain (retention lowered so almost the whole partition is deletable) a single scan on a multi-million-batch partition runs for seconds-to-minutes and exceeds the enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the still-running backend so the partition never drains. V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The boundary can never be deeper than the cap because the delete is clamped there regardless; the +1 distinguishes an in-window boundary (delete up to it) from a deeper one that falls through to high_watermark and is re-clamped by the existing under-lock cap-probe. Each pass is O(cap), index-only on batches_by_last_offset_covering_idx, and a deep partition drains over successive enforcement cycles. max_batches_per_request = 0 keeps the original unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes oldest-first, so a snapshot-stale boundary can only under-delete). Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the bound is on out of the box; it must stay above the per-interval expiry rate or retention falls behind. Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via -Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms (flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is the sweet spot: short passes at 10x the drain throughput of 100. [KC-354]
jeqo
added a commit
that referenced
this pull request
Jul 28, 2026
…O(cap) The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every deletable batch, O(partition depth). PR #705 moved it out of the log lock so it no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain (retention lowered so almost the whole partition is deletable) a single scan on a multi-million-batch partition runs for seconds-to-minutes and exceeds the enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the still-running backend so the partition never drains. V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The boundary can never be deeper than the cap because the delete is clamped there regardless; the +1 distinguishes an in-window boundary (delete up to it) from a deeper one that falls through to high_watermark and is re-clamped by the existing under-lock cap-probe. Each pass is O(cap), index-only on batches_by_last_offset_covering_idx, and a deep partition drains over successive enforcement cycles. max_batches_per_request = 0 keeps the original unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes oldest-first, so a snapshot-stale boundary can only under-delete). Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the bound is on out of the box; it must stay above the per-interval expiry rate or retention falls behind. Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via -Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms (flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is the sweet spot: short passes at 10x the drain throughput of 100. [KC-354]
viktorsomogyi
pushed a commit
that referenced
this pull request
Jul 28, 2026
…O(cap) (#722) The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every deletable batch, O(partition depth). PR #705 moved it out of the log lock so it no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain (retention lowered so almost the whole partition is deletable) a single scan on a multi-million-batch partition runs for seconds-to-minutes and exceeds the enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the still-running backend so the partition never drains. V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The boundary can never be deeper than the cap because the delete is clamped there regardless; the +1 distinguishes an in-window boundary (delete up to it) from a deeper one that falls through to high_watermark and is re-clamped by the existing under-lock cap-probe. Each pass is O(cap), index-only on batches_by_last_offset_covering_idx, and a deep partition drains over successive enforcement cycles. max_batches_per_request = 0 keeps the original unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes oldest-first, so a snapshot-stale boundary can only under-delete). Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the bound is on out of the box; it must stay above the per-interval expiry rate or retention falls behind. Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via -Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms (flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is the sweet spot: short passes at 10x the drain throughput of 100. [KC-354]
jeqo
added a commit
that referenced
this pull request
Jul 28, 2026
EnforceRetentionJob runs one transaction per partition (post-#705), yet EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side RetentionEnforcementTotalTime, which already measures the whole enforce wave. Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per partition) instead, matching the other *QueryTime metrics (one DB operation each) and yielding per-partition enforce latency without a new metric. The whole-wave total stays RetentionEnforcementTotalTime on the broker. Note: this redefines two existing metrics -- EnforceRetentionQueryTime from per-wave to per-partition, and EnforceRetentionQueryRate from wave count to partitions-enforced count (wave count remains RetentionEnforcementRate).
jeqo
added a commit
that referenced
this pull request
Jul 28, 2026
EnforceRetentionJob runs one transaction per partition (post-#705), yet EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side RetentionEnforcementTotalTime, which already measures the whole enforce wave. Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per partition) instead, matching the other *QueryTime metrics (one DB operation each) and yielding per-partition enforce latency without a new metric. The whole-wave total stays RetentionEnforcementTotalTime on the broker. Note: this redefines two existing metrics -- EnforceRetentionQueryTime from per-wave to per-partition, and EnforceRetentionQueryRate from wave count to partitions-enforced count (wave count remains RetentionEnforcementRate).
giuseppelillo
pushed a commit
that referenced
this pull request
Jul 29, 2026
…#725) * feat(inkless:retention): surface enforcement schedule lag Add a RetentionEnforcementScheduleLagMs gauge (0 when on schedule; a sustained positive value means enforcement is falling behind its per-partition cadence), fed by the scheduler's oldest past-due queue entry read under a lock so the metrics/JMX thread and the enforcement thread do not race the PriorityQueue. * feat(inkless:retention): meter enforce latency per partition EnforceRetentionJob runs one transaction per partition (post-#705), yet EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side RetentionEnforcementTotalTime, which already measures the whole enforce wave. Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per partition) instead, matching the other *QueryTime metrics (one DB operation each) and yielding per-partition enforce latency without a new metric. The whole-wave total stays RetentionEnforcementTotalTime on the broker. Note: this redefines two existing metrics -- EnforceRetentionQueryTime from per-wave to per-partition, and EnforceRetentionQueryRate from wave count to partitions-enforced count (wave count remains RetentionEnforcementRate). * feat(inkless:retention): make enforcement and file-cleaner logs intent/result pairs Retention enforcer: promote the per-cycle summary from DEBUG to INFO and add a matching intent line before the (potentially blocking) enforce call, so a stuck enforce shows as intent-without-completion: "Enforcing retention for {n} partitions" "Enforced retention for {n} partitions in {ms} ms: {b} batches, {y} bytes deleted" One pair per non-empty cycle; empty cycles stay silent (no per-tick heartbeat). Also fix a latent partition misattribution: the per-partition log lines indexed readyPartitions while responses align with the delete-policy-filtered requests, so a filtered-out partition could be mislabeled -- keep an enforcedPartitions list aligned 1:1 with requests/responses instead. File cleaner: replace the per-run "Running file cleaner at {now}" heartbeat (redundant with the framework timestamp and the outcome line) with a work-gated intent/result pair: "Running file cleaner: deleting {n} of {m} marked files" (m-n still within grace) "File cleaner deleted {n} files in {ms} ms" Partition/size counts are not available at this layer (FileToDelete carries only the object key; batches are already deleted by cleanup time).
giuseppelillo
pushed a commit
that referenced
this pull request
Jul 29, 2026
…O(cap) (#722) The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every deletable batch, O(partition depth). PR #705 moved it out of the log lock so it no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain (retention lowered so almost the whole partition is deletable) a single scan on a multi-million-batch partition runs for seconds-to-minutes and exceeds the enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the still-running backend so the partition never drains. V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The boundary can never be deeper than the cap because the delete is clamped there regardless; the +1 distinguishes an in-window boundary (delete up to it) from a deeper one that falls through to high_watermark and is re-clamped by the existing under-lock cap-probe. Each pass is O(cap), index-only on batches_by_last_offset_covering_idx, and a deep partition drains over successive enforcement cycles. max_batches_per_request = 0 keeps the original unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes oldest-first, so a snapshot-stale boundary can only under-delete). Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the bound is on out of the box; it must stay above the per-interval expiry rate or retention falls behind. Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via -Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms (flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is the sweet spot: short passes at 10x the drain throughput of 100. [KC-354]
giuseppelillo
pushed a commit
that referenced
this pull request
Jul 29, 2026
…#725) * feat(inkless:retention): surface enforcement schedule lag Add a RetentionEnforcementScheduleLagMs gauge (0 when on schedule; a sustained positive value means enforcement is falling behind its per-partition cadence), fed by the scheduler's oldest past-due queue entry read under a lock so the metrics/JMX thread and the enforcement thread do not race the PriorityQueue. * feat(inkless:retention): meter enforce latency per partition EnforceRetentionJob runs one transaction per partition (post-#705), yet EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side RetentionEnforcementTotalTime, which already measures the whole enforce wave. Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per partition) instead, matching the other *QueryTime metrics (one DB operation each) and yielding per-partition enforce latency without a new metric. The whole-wave total stays RetentionEnforcementTotalTime on the broker. Note: this redefines two existing metrics -- EnforceRetentionQueryTime from per-wave to per-partition, and EnforceRetentionQueryRate from wave count to partitions-enforced count (wave count remains RetentionEnforcementRate). * feat(inkless:retention): make enforcement and file-cleaner logs intent/result pairs Retention enforcer: promote the per-cycle summary from DEBUG to INFO and add a matching intent line before the (potentially blocking) enforce call, so a stuck enforce shows as intent-without-completion: "Enforcing retention for {n} partitions" "Enforced retention for {n} partitions in {ms} ms: {b} batches, {y} bytes deleted" One pair per non-empty cycle; empty cycles stay silent (no per-tick heartbeat). Also fix a latent partition misattribution: the per-partition log lines indexed readyPartitions while responses align with the delete-policy-filtered requests, so a filtered-out partition could be mislabeled -- keep an enforcedPartitions list aligned 1:1 with requests/responses instead. File cleaner: replace the per-run "Running file cleaner at {now}" heartbeat (redundant with the framework timestamp and the outcome line) with a work-gated intent/result pair: "Running file cleaner: deleting {n} of {m} marked files" (m-n still within grace) "File cleaner deleted {n} files in {ms} ms" Partition/size counts are not available at this layer (FileToDelete carries only the object key; batches are already deleted by cleanup time).
giuseppelillo
pushed a commit
that referenced
this pull request
Jul 30, 2026
…O(cap) (#722) The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every deletable batch, O(partition depth). PR #705 moved it out of the log lock so it no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain (retention lowered so almost the whole partition is deletable) a single scan on a multi-million-batch partition runs for seconds-to-minutes and exceeds the enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the still-running backend so the partition never drains. V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The boundary can never be deeper than the cap because the delete is clamped there regardless; the +1 distinguishes an in-window boundary (delete up to it) from a deeper one that falls through to high_watermark and is re-clamped by the existing under-lock cap-probe. Each pass is O(cap), index-only on batches_by_last_offset_covering_idx, and a deep partition drains over successive enforcement cycles. max_batches_per_request = 0 keeps the original unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes oldest-first, so a snapshot-stale boundary can only under-delete). Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the bound is on out of the box; it must stay above the per-interval expiry rate or retention falls behind. Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via -Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms (flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is the sweet spot: short passes at 10x the drain throughput of 100. [KC-354]
giuseppelillo
pushed a commit
that referenced
this pull request
Jul 30, 2026
…#725) * feat(inkless:retention): surface enforcement schedule lag Add a RetentionEnforcementScheduleLagMs gauge (0 when on schedule; a sustained positive value means enforcement is falling behind its per-partition cadence), fed by the scheduler's oldest past-due queue entry read under a lock so the metrics/JMX thread and the enforcement thread do not race the PriorityQueue. * feat(inkless:retention): meter enforce latency per partition EnforceRetentionJob runs one transaction per partition (post-#705), yet EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side RetentionEnforcementTotalTime, which already measures the whole enforce wave. Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per partition) instead, matching the other *QueryTime metrics (one DB operation each) and yielding per-partition enforce latency without a new metric. The whole-wave total stays RetentionEnforcementTotalTime on the broker. Note: this redefines two existing metrics -- EnforceRetentionQueryTime from per-wave to per-partition, and EnforceRetentionQueryRate from wave count to partitions-enforced count (wave count remains RetentionEnforcementRate). * feat(inkless:retention): make enforcement and file-cleaner logs intent/result pairs Retention enforcer: promote the per-cycle summary from DEBUG to INFO and add a matching intent line before the (potentially blocking) enforce call, so a stuck enforce shows as intent-without-completion: "Enforcing retention for {n} partitions" "Enforced retention for {n} partitions in {ms} ms: {b} batches, {y} bytes deleted" One pair per non-empty cycle; empty cycles stay silent (no per-tick heartbeat). Also fix a latent partition misattribution: the per-partition log lines indexed readyPartitions while responses align with the delete-policy-filtered requests, so a filtered-out partition could be mislabeled -- keep an enforcedPartitions list aligned 1:1 with requests/responses instead. File cleaner: replace the per-run "Running file cleaner at {now}" heartbeat (redundant with the framework timestamp and the outcome line) with a work-gated intent/result pair: "Running file cleaner: deleting {n} of {m} marked files" (m-n still within grace) "File cleaner deleted {n} files in {ms} ms" Partition/size counts are not available at this layer (FileToDelete carries only the object key; batches are already deleted by cleanup time).
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.
enforce_retention_v2tooklogs FOR UPDATEbefore its O(depth) retention boundary scan and looped every request in a single transaction. A concurrentcommit_fileon a locked partition therefore waited for the whole enforcement pass, and in production the commit-latency tail tracked the enforce latency (commit max was roughly equal to enforce max).Change
Enforcement is restructured so the lock is held only for the short delete, not the scan:
FOR UPDATE.byte_sizeis read inside that scan statement (aselected_logCTE) so the reverse-aggregate stays consistent with thebatchesrows under READ COMMITTED. The lock is taken only afterwards, for the delete.EnforceRetentionJobruns one transaction per request instead of wrapping all requests in one. A plpgsql function cannot release a row lock before it returns, so per-request transactions are what actually free the lock between partitions rather than accumulating it until the end.nowis computed once per cycle and shared. Each transaction holds at most one partition lock, so enforce can no longer self-deadlock.log_startbetween the unlocked scan and the locked delete.max_batches_per_requestcap probes for the(cap+1)-th deletable batch withLIMIT/OFFSETinstead ofCOUNT(*)over all deletable rows, keeping the locked section O(delete-size) rather than O(depth) when a small cap meets a large deletable set.logs.byte_sizeandlogs.earliest_batch_timestamp. ANULLearliest_batch_timestampmeans "unknown", so the short-circuit does not fire and the scan runs.Safety
Computing the boundary unlocked is safe: retention deletes at the tail while commit appends at the head, and
delete_records_v1re-clamps to the currentlog_startunder its own lock, so a concurrent commit or delete cannot cause over-deletion. Partial progress across partitions is fine; retention is idempotent and re-runs next cycle.Results
Indicative local numbers (testcontainers, single machine) from the committed
@Tag("benchmark")harness,maxBatchesPerRequest=1so delete work is tiny and the measured tail is lock coupling.max/enforceis the concurrent commit's max latency as a fraction of the enforce latency;commitsis how many commits completed during one enforce pass.Single partition, commit to the same partition being enforced:
Multi-partition (8 partitions enforced in one cycle), commit to the first partition:
Commit p50/p95 during enforcement drop to roughly 1-5 ms, and commit max no longer scales with partition depth.
Testing
PostgresControlPlaneTest, including theRetentionsize/time suites) and the in-memory contract remain green; the SQL semantics are unchanged.EnforceRetentionJobTest: log deleted between scan and delete returnsunknown_topic_or_partition;log_startadvanced between scan and delete returns the recounted deletion; and aNULLearliest_batch_timestampfalls back to the scan instead of short-circuiting.Stacking / dependencies
Stacked on the
find_batchesbounded-scan change and on thelogs.earliest_batch_timestampchange (the short-circuit reads that column, so this must apply after it).Out of scope (follow-ups)
FOR UPDATE SKIP LOCKEDwith a fairness backstop, if a residual enforce-vs-commit tail remains after this change.