Repository navigation
fix(inkless:storage): drain object-store deletes monotonically under throttling [KC-413] - #715
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Inkless storage deletion semantics to support monotonic draining under object-store throttling: storage backends now report which keys were confirmed deleted so FileCleaner can advance control-plane dereferences on partial progress instead of reissuing redundant deletes.
Changes:
- Change
ObjectDeleter.delete(Set)to return the subset of keys confirmed deleted (idempotent: already-absent counts as deleted). - Update
FileCleanerto dereference only confirmed-deleted keys and apply backoff when a cycle drains nothing. - Update S3/Azure/In-memory/test backends to implement partial-progress deletion behavior and add/adjust tests for the new contract and throttling scenarios.
Reviewed changes
Copilot reviewed 9 out of 9 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/storage_backend/common/ObjectDeleter.java | Changes the multi-key delete contract to return confirmed-deleted keys. |
| storage/inkless/src/main/java/io/aiven/inkless/delete/FileCleaner.java | Uses partial deletion results to dereference only confirmed-deleted keys; backs off when no progress is made. |
| storage/inkless/src/main/java/io/aiven/inkless/storage_backend/s3/S3Storage.java | Implements single-pass S3 batch delete that returns confirmed deletions and logs per-key/whole-request failures without aborting progress. |
| storage/inkless/src/main/java/io/aiven/inkless/storage_backend/azure/AzureBlobStorage.java | Accumulates and returns per-blob confirmed deletions instead of aborting on first failure. |
| storage/inkless/src/main/java/io/aiven/inkless/storage_backend/gcs/GcsStorage.java | Propagates new return type while keeping batch delete effectively all-or-nothing on thrown failures. |
| storage/inkless/src/main/java/io/aiven/inkless/storage_backend/in_memory/InMemoryStorage.java | Propagates new return type (returns input set on success). |
| storage/inkless/src/test/java/io/aiven/inkless/config/ConfigTestStorageBackend.java | Propagates new return type for config-test backend. |
| storage/inkless/src/test/java/io/aiven/inkless/delete/FileCleanerMockedTest.java | Adds tests for monotonic draining, no-progress backoff, and storage-failure handling; updates stubs for new return type. |
| storage/inkless/src/test/java/io/aiven/inkless/storage_backend/s3/integration/S3StorageDeleteTest.java | Adds WireMock integration coverage for partial deletions and whole-request failure behavior. |
jeqo
marked this pull request as ready for review
July 22, 2026 18:30
jeqo
marked this pull request as draft
August 6, 2026 21:26
8000
…throttling Under S3 throttling the FileCleaner cycle aborted entirely: S3Storage.delete threw on the first per-key or whole-request error, so FileCleaner never dereferenced the files it had already deleted. The next cycle re-fetched the same set and re-issued DeleteObjects for objects that were already gone, looping without draining and amplifying the throttle. Change the delete contract so partial progress is preserved: - ObjectDeleter.delete(Set) now returns the subset of keys confirmed deleted (idempotent: already-absent keys count as deleted) instead of void. The in-memory and config-test backends return the input set on success. - S3Storage.delete does a single pass per 1000-key batch: it collects the keys S3 confirmed deleted (DeleteObjectsResponse.deleted()) and logs the rest, distinguishing throttling from hard errors for diagnostics only. It no longer throws for partial or whole-request failures (incl. a 503 the SDK's adaptive retry exhausted); undeleted keys stay marked for deletion and are retried on the next cycle. - AzureBlobStorage.delete deletes one blob at a time, so it now accumulates and returns the confirmed-deleted subset instead of throwing on the first failure and abandoning the rest (same monotonic-drain benefit as S3). - GcsStorage.delete stays all-or-nothing: Storage.delete(Iterable) is an atomic batch that raises a genuine failure as a thrown exception rather than a per-blob flag, so a confirmed-deleted subset cannot be extracted; a thrown batch drains nothing and the whole set is retried next cycle (documented inline). - FileCleaner dereferences only the confirmed-deleted subset in the control plane and records that count, so the set shrinks monotonically instead of re-attempting already-deleted keys. Request-rate backoff stays where it already lives: the S3 client's ADAPTIVE_V2 retry strategy within a cycle, and the file-cleaner schedule between cycles. No in-run backoff is added here. No control-plane change is needed: deleteFiles already accepts any subset. Tests: FileCleanerMockedTest gains partial-drain, nothing-deleted, and storage-failure cases, and the existing cases stub the new return value; new S3StorageDeleteTest covers single-pass partial return without retry and a whole-request failure that must not propagate; Azure/GCS are covered by the existing BaseStorageTest.testDeletes integration tests.
Partial delete progress is now the normal case: the delete contract returns the subset of keys the backend confirmed, and a throttled or rejected key stays marked for deletion instead of throwing. Nothing reports that. FileCleanerErrorRate counts thrown exceptions, which the delete path no longer raises for partial failure, and FileCleanerFilesRate counts only what succeeded - so a cycle that drains half its worklist is indistinguishable from one that drains all of it, cycle after cycle. Add FileCleanerFilesFailedRate, the difference between the keys submitted and the keys confirmed. A sustained non-zero value is the signal that keys are being re-attempted every cycle rather than draining, which is what distinguishes throttling (recovers on its own) from a hard per-key failure (does not). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
storage/inkless/src/main/java/io/aiven/inkless/storage_backend/azure/AzureBlobStorage.java:217
- The existing
BaseStorageTest.testDeletesonly exercises an all-success call and discards the return value, so it does not cover this new partial-progress behavior. Add a focused test where one Azure deletion fails and a later one succeeds, asserting that the failed key is omitted and iteration continues; otherwise a regression back to aborting the batch would go undetected.
} catch (final BlobStorageException e) {
LOGGER.warn("Failed to delete {}; leaving it for the next cycle", key, e);
} catch (final RuntimeException e) {
LOGGER.warn("Failed to delete {}; leaving it for the next cycle", key, Exceptions.unwrap(e));
storage/inkless/src/main/java/io/aiven/inkless/storage_backend/gcs/GcsStorage.java:137
Storage.delete(Iterable)may have deleted some blobs before one batched result raises, so “on failure we delete nothing” is not guaranteed. The implementation safely retries the entire set because deletion is idempotent, but the comment should describe what is reported rather than claim no physical deletion occurred.
// cannot extract a confirmed-deleted subset the way the S3 backend does. This stays
// all-or-nothing: on success every key is gone (idempotent), and on failure we delete
// nothing and let the FileCleaner cycle retry the whole set.
jeqo
marked this pull request as ready for review
August 13, 2026 07:57
jeqo
added a commit
that referenced
this pull request
Aug 19, 2026
…throttling [KC-413] (#715) * fix(inkless:storage): drain object-store deletes monotonically under throttling Under S3 throttling the FileCleaner cycle aborted entirely: S3Storage.delete threw on the first per-key or whole-request error, so FileCleaner never dereferenced the files it had already deleted. The next cycle re-fetched the same set and re-issued DeleteObjects for objects that were already gone, looping without draining and amplifying the throttle. Change the delete contract so partial progress is preserved: - ObjectDeleter.delete(Set) now returns the subset of keys confirmed deleted (idempotent: already-absent keys count as deleted) instead of void. The in-memory and config-test backends return the input set on success. - S3Storage.delete does a single pass per 1000-key batch: it collects the keys S3 confirmed deleted (DeleteObjectsResponse.deleted()) and logs the rest, distinguishing throttling from hard errors for diagnostics only. It no longer throws for partial or whole-request failures (incl. a 503 the SDK's adaptive retry exhausted); undeleted keys stay marked for deletion and are retried on the next cycle. - AzureBlobStorage.delete deletes one blob at a time, so it now accumulates and returns the confirmed-deleted subset instead of throwing on the first failure and abandoning the rest (same monotonic-drain benefit as S3). - GcsStorage.delete stays all-or-nothing: Storage.delete(Iterable) is an atomic batch that raises a genuine failure as a thrown exception rather than a per-blob flag, so a confirmed-deleted subset cannot be extracted; a thrown batch drains nothing and the whole set is retried next cycle (documented inline). - FileCleaner dereferences only the confirmed-deleted subset in the control plane and records that count, so the set shrinks monotonically instead of re-attempting already-deleted keys. Request-rate backoff stays where it already lives: the S3 client's ADAPTIVE_V2 retry strategy within a cycle, and the file-cleaner schedule between cycles. No in-run backoff is added here. No control-plane change is needed: deleteFiles already accepts any subset. Tests: FileCleanerMockedTest gains partial-drain, nothing-deleted, and storage-failure cases, and the existing cases stub the new return value; new S3StorageDeleteTest covers single-pass partial return without retry and a whole-request failure that must not propagate; Azure/GCS are covered by the existing BaseStorageTest.testDeletes integration tests. * feat(inkless:storage): count files the backend did not confirm deleted Partial delete progress is now the normal case: the delete contract returns the subset of keys the backend confirmed, and a throttled or rejected key stays marked for deletion instead of throwing. Nothing reports that. FileCleanerErrorRate counts thrown exceptions, which the delete path no longer raises for partial failure, and FileCleanerFilesRate counts only what succeeded - so a cycle that drains half its worklist is indistinguishable from one that drains all of it, cycle after cycle. Add FileCleanerFilesFailedRate, the difference between the keys submitted and the keys confirmed. A sustained non-zero value is the signal that keys are being re-attempted every cycle rather than draining, which is what distinguishes throttling (recovers on its own) from a hard per-key failure (does not). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
jeqo
added a commit
that referenced
this pull request
Aug 19, 2026
…throttling [KC-413] (#715) * fix(inkless:storage): drain object-store deletes monotonically under throttling Under S3 throttling the FileCleaner cycle aborted entirely: S3Storage.delete threw on the first per-key or whole-request error, so FileCleaner never dereferenced the files it had already deleted. The next cycle re-fetched the same set and re-issued DeleteObjects for objects that were already gone, looping without draining and amplifying the throttle. Change the delete contract so partial progress is preserved: - ObjectDeleter.delete(Set) now returns the subset of keys confirmed deleted (idempotent: already-absent keys count as deleted) instead of void. The in-memory and config-test backends return the input set on success. - S3Storage.delete does a single pass per 1000-key batch: it collects the keys S3 confirmed deleted (DeleteObjectsResponse.deleted()) and logs the rest, distinguishing throttling from hard errors for diagnostics only. It no longer throws for partial or whole-request failures (incl. a 503 the SDK's adaptive retry exhausted); undeleted keys stay marked for deletion and are retried on the next cycle. - AzureBlobStorage.delete deletes one blob at a time, so it now accumulates and returns the confirmed-deleted subset instead of throwing on the first failure and abandoning the rest (same monotonic-drain benefit as S3). - GcsStorage.delete stays all-or-nothing: Storage.delete(Iterable) is an atomic batch that raises a genuine failure as a thrown exception rather than a per-blob flag, so a confirmed-deleted subset cannot be extracted; a thrown batch drains nothing and the whole set is retried next cycle (documented inline). - FileCleaner dereferences only the confirmed-deleted subset in the control plane and records that count, so the set shrinks monotonically instead of re-attempting already-deleted keys. Request-rate backoff stays where it already lives: the S3 client's ADAPTIVE_V2 retry strategy within a cycle, and the file-cleaner schedule between cycles. No in-run backoff is added here. No control-plane change is needed: deleteFiles already accepts any subset. Tests: FileCleanerMockedTest gains partial-drain, nothing-deleted, and storage-failure cases, and the existing cases stub the new return value; new S3StorageDeleteTest covers single-pass partial return without retry and a whole-request failure that must not propagate; Azure/GCS are covered by the existing BaseStorageTest.testDeletes integration tests. * feat(inkless:storage): count files the backend did not confirm deleted Partial delete progress is now the normal case: the delete contract returns the subset of keys the backend confirmed, and a throttled or rejected key stays marked for deletion instead of throwing. Nothing reports that. FileCleanerErrorRate counts thrown exceptions, which the delete path no longer raises for partial failure, and FileCleanerFilesRate counts only what succeeded - so a cycle that drains half its worklist is indistinguishable from one that drains all of it, cycle after cycle. Add FileCleanerFilesFailedRate, the difference between the keys submitted and the keys confirmed. A sustained non-zero value is the signal that keys are being re-attempted every cycle rather than draining, which is what distinguishes throttling (recovers on its own) from a hard per-key failure (does not). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
jeqo
added a commit
that referenced
this pull request
Aug 19, 2026
Covers inkless-release-0.46..inkless-release-0.47. Generated via the inkless-changelog skill and curated: - Metric deltas cross-checked against source commits (#716, #735, #743, #746, #715); all trace to real, shipped metrics, no documentation catch-up noise this increment. - Postgres migration headers (V23-V26) checked for lock impact; V25 takes ACCESS EXCLUSIVE on files for a full-table scan at broker startup, V26 offers a pre-upgrade CONCURRENTLY workaround. - Added two config default changes the generator's diff-only detection missed (existing keys, not new ones): file.cleaner.interval.ms (5min -> 2min, #745) and retention.enforcement.interval.ms / max.batches.per.request (#744).
jeqo
added a commit
that referenced
this pull request
Sep 9, 2026
Covers inkless-release-0.46..inkless-release-0.47. Generated via the inkless-changelog skill and curated: - Metric deltas cross-checked against source commits (#716, #735, #743, #746, #715); all trace to real, shipped metrics, no documentation catch-up noise this increment. - Postgres migration headers (V23-V26) checked for lock impact; V25 takes ACCESS EXCLUSIVE on files for a full-table scan at broker startup, V26 offers a pre-upgrade CONCURRENTLY workaround. - Added two config default changes the generator's diff-only detection missed (existing keys, not new ones): file.cleaner.interval.ms (5min -> 2min, #745) and retention.enforcement.interval.ms / max.batches.per.request (#744).
jeqo
added a commit
that referenced
this pull request
Sep 9, 2026
Covers inkless-release-0.46..inkless-release-0.47. Generated via the inkless-changelog skill and curated: - Metric deltas cross-checked against source commits (#716, #735, #743, #746, #715); all trace to real, shipped metrics, no documentation catch-up noise this increment. - Postgres migration headers (V23-V26) checked for lock impact; V25 takes ACCESS EXCLUSIVE on files for a full-table scan at broker startup, V26 offers a pre-upgrade CONCURRENTLY workaround. - Added two config default changes the generator's diff-only detection missed (existing keys, not new ones): file.cleaner.interval.ms (5min -> 2min, #745) and retention.enforcement.interval.ms / max.batches.per.request (#744).
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.
When object storage throttled deletes, the WAL file cleaner aborted the whole cycle and re-attempted keys it had already deleted, looping without draining and amplifying the throttle. This makes deletion drain monotonically: only the keys the backend confirms deleted are dereferenced, and the rest are retried on the next cycle.
Problem
S3Storage.delete(Set)threw on the first per-key or whole-request error. SinceFileCleanerdeletes from storage before updating the control plane, that throw dereferenced nothing for the whole batch — including keys S3 had already deleted in earlier slices. The next cycle re-issuedDeleteObjectsfor objects that were already gone, so under sustained throttling the set never drained.Change
ObjectDeleter.delete(Set)now returns the subset of keys confirmed deleted (already-absent counts as deleted) instead ofvoid, so callers can advance on partial progress.FileCleanerdereferences only that subset; undeleted keys stay marked and are retried by the next periodic cycle.DeleteObjectsResponse.deleted()and logging the rest. No longer throws on partial or whole-request failure (including a 503 the SDK's adaptive retry exhausted).Storage.delete(Iterable)reports batch failure as a thrown exception, so no confirmed subset can be extracted.No control-plane change needed:
deleteFilesalready accepts any subset of keys.Observability
Partial progress is now the normal case, and nothing reported it:
FileCleanerErrorRatecounts thrown exceptions, which partial failure no longer raises, andFileCleanerFilesRatecounts only what succeeded — so a cycle draining half its worklist looked identical to one draining all of it. NewFileCleanerFilesFailedRatecounts submitted minus confirmed; a sustained non-zero value means keys are being re-attempted every cycle rather than draining, which separates throttling (self-recovering) from a hard per-key failure.Testing
FileCleanerMockedTest: partial drain, no progress, and storage failure, plus the failed-files count.S3StorageDeleteTest(WireMock): partial return without retry, and a whole-request 503 that must not propagate.BaseStorageTest.testDeletesagainst Azurite / fake-gcs.