Skip to content

Commit ffcad22

Browse files
jeqoclaude
jeqo
and
claude
committed
fix(inkless:storage): bound bulk-delete failure logging
A delete pass that fails for every key it submitted logged one line per key (with a stack trace per key on Azure) and repeated it on every cleanup cycle, since undeleted keys stay marked for deletion. GCS still interpolated the whole key set into the exception message, the bug #733 fixed for S3. Aggregate per-key failures into DeleteErrorSummary: a count by error code, at most 3 sampled hard failures, and the first hard cause. One line per call, bounded by the distinct error codes rather than the key count, at INFO when every failure was a throttle and WARN otherwise. GCS cannot report per-key results, so its message just carries the key count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 894fa9c commit ffcad22

5 files changed

Lines changed: 207 additions & 21 deletions

File tree

‎storage/inkless/src/main/java/io/aiven/inkless/storage_backend/azure/AzureBlobStorage.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848

4949
import io.aiven.inkless.common.ByteRange;
5050
import io.aiven.inkless.common.ObjectKey;
51+
import io.aiven.inkless.storage_backend.common.DeleteErrorSummary;
5152
import io.aiven.inkless.storage_backend.common.InvalidRangeException;
5253
import io.aiven.inkless.storage_backend.common.KeyNotFoundException;
5354
import io.aiven.inkless.storage_backend.common.StorageBackend;
@@ -58,6 +59,12 @@
5859
public final class AzureBlobStorage extends StorageBackend {
5960
private static final Logger LOGGER = LoggerFactory.getLogger(AzureBlobStorage.class);
6061

62+
// Azure error codes that indicate throttling or a transient server condition rather than a hard,
63+
// non-transient failure. Used only to log throttling distinctly; both kinds are left for the next
64+
// FileCleaner cycle to retry.
65+
private static final Set<String> THROTTLE_ERROR_CODES =
66+
Set.of("ServerBusy", "InternalError", "OperationTimedOut");
67+
6168
private AzureBlobStorageConfig config;
6269
private BlobContainerClient blobContainerClient;
6370
private MetricCollector.MetricsPolicy policy;
@@ -207,16 +214,21 @@ public Set<ObjectKey> delete(final Set<ObjectKey> keys) throws StorageBackendExc
207214
// failed ones as not deleted. deleteIfExists() returns true if the blob was deleted and false
208215
// if it was already absent; both mean the key is gone (idempotent).
209216
final Set<ObjectKey> deleted = new HashSet<>();
217+
final DeleteErrorSummary errors = new DeleteErrorSummary();
210218
for (final ObjectKey key : keys) {
211219
try {
212220
blobContainerClient.getBlobClient(key.value()).deleteIfExists();
213221
deleted.add(key);
214222
} catch (final BlobStorageException e) {
215-
LOGGER.warn("Failed to delete {}; leaving it for the next cycle", key, e);
223+
final String code = String.valueOf(e.getErrorCode());
224+
errors.record(code, key + ": " + e.getMessage(), THROTTLE_ERROR_CODES.contains(code), e);
216225
} catch (final RuntimeException e) {
217-
LOGGER.warn("Failed to delete {}; leaving it for the next cycle", key, Exceptions.unwrap(e));
226+
final Throwable unwrapped = Exceptions.unwrap(e);
227+
errors.record(unwrapped.getClass().getSimpleName(),
228+
key + ": " + unwrapped.getMessage(), false, unwrapped);
218229
}
219230
}
231+
errors.log(LOGGER, keys.size(), deleted.size());
220232
return deleted;
221233
}
222234

‎storage/inkless/src/main/java/io/aiven/inkless/storage_backend/common/DeleteErrorSummary.java‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
/*
2+
* Inkless
3+
* Copyright (C) 2024 - 2026 Aiven OY
4+
*
5+
* This program is free software: you can redistribute it and/or modify
6+
* it under the terms of the GNU Affero General Public License as published by
7+
* the Free Software Foundation, either version 3 of the License, or
8+
* (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU Affero General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Affero General Public License
16+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
17+
*/
18+
package io.aiven.inkless.storage_backend.common;
19+
20+
import org.slf4j.Logger;
21+
22+
import java.util.ArrayList;
23+
import java.util.List;
24+
import java.util.Map;
25+
import java.util.Optional;
26+
import java.util.TreeMap;
27+
28+
/**
29+
* Accumulates the per-key failures of one bulk delete into a single log line, bounded by the number of
30+
* distinct error codes rather than by the number of keys submitted.
31+
*
32+
* <p>Throttled keys are counted but never sampled: they carry no per-key information and dominate the
33+
* failures under load. A call that failed only from throttling is reported at INFO, since it is normal
34+
* backpressure rather than an operator-actionable failure; anything else is reported at WARN.
35+
*/
36+
public final class DeleteErrorSummary {
37+
38+
private static final int MAX_SAMPLES = 3;
39+
40+
private final Map<String, Integer> countsByCode = new TreeMap<>();
41+
private final List<String> samples = new ArrayList<>();
42+
private Throwable firstHardFailure;
43+
44+
public void record(final String errorCode, final String keyDetail, final boolean throttled) {
45+
record(errorCode, keyDetail, throttled, null);
46+
}
47+
48+
/**
49+
* @param cause reported with the summary line for the first hard failure only, so that one stack
50+
* trace per delete call reaches the log instead of one per key.
51+
*/
52+
public void record(final String errorCode,
53+
final String keyDetail,
54+
final boolean throttled,
55+
final Throwable cause) {
56+
countsByCode.merge(errorCode, 1, Integer::sum);
57+
if (throttled) {
58+
return;
59+
}
60+
if (samples.size() < MAX_SAMPLES) {
61+
samples.add(keyDetail);
62+
}
63+
if (firstHardFailure == null) {
64+
firstHardFailure = cause;
65+
}
66+
}
67+
68+
public boolean isEmpty() {
69+
return countsByCode.isEmpty();
70+
}
71+
72+
public void log(final Logger logger, final int requested, final int deletedCount) {
73+
if (isEmpty()) {
74+
return;
75+
}
76+
final String summary = format(requested, deletedCount);
77+
if (samples.isEmpty()) {
78+
logger.info(summary);
79+
} else if (firstHardFailure != null) {
80+
logger.warn(summary, firstHardFailure);
81+
} else {
82+
logger.warn(summary);
83+
}
84+
}
85+
86+
// visible for testing
87+
String format(final int requested, final int deletedCount) {
88+
return String.format(
89+
"Failed to delete %d of %d keys %s%s",
90+
requested - deletedCount,
91+
requested,
92+
countsByCode,
93+
samples.isEmpty() ? "" : "; e.g. " + String.join("; ", samples)
94+
);
95+
}
96+
97+
// visible for testing
98+
Optional<Throwable> firstHardFailure() {
99+
return Optional.ofNullable(firstHardFailure);
100+
}
101+
}

‎storage/inkless/src/main/java/io/aiven/inkless/storage_backend/gcs/GcsStorage.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ public Set<ObjectKey> delete(final Set<ObjectKey> keys) throws StorageBackendExc
138138
storage.delete(ids);
139139
return Set.copyOf(keys);
140140
} catch (final BaseServiceException e) {
141-
throw new StorageBackendException("Failed to delete " + keys, e);
141+
throw new StorageBackendException("Failed to delete " + keys.size() + " keys", e);
142142
}
143143
}
144144

‎storage/inkless/src/main/java/io/aiven/inkless/storage_backend/s3/S3Storage.java‎

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939

4040
import io.aiven.inkless.common.ByteRange;
4141
import io.aiven.inkless.common.ObjectKey;
42+
import io.aiven.inkless.storage_backend.common.DeleteErrorSummary;
4243
import io.aiven.inkless.storage_backend.common.InvalidRangeException;
4344
import io.aiven.inkless.storage_backend.common.KeyNotFoundException;
4445
import io.aiven.inkless.storage_backend.common.StorageBackend;
@@ -171,6 +172,7 @@ public void delete(final ObjectKey key) throws StorageBackendException {
171172
public Set<ObjectKey> delete(final Set<ObjectKey> keys) throws StorageBackendException {
172173
final List<ObjectKey> objectKeys = new ArrayList<>(keys);
173174
final Set<ObjectKey> deleted = new HashSet<>();
175+
final DeleteErrorSummary errors = new DeleteErrorSummary();
174176
for (int i = 0; i < objectKeys.size(); i += MAX_DELETE_KEYS_LIMIT) {
175177
final Set<ObjectKey> batch = new HashSet<>(objectKeys.subList(
176178
i,
@@ -196,29 +198,19 @@ public Set<ObjectKey> delete(final Set<ObjectKey> keys) throws StorageBackendExc
196198
deleted.add(key);
197199
}
198200
}
199-
logDeleteErrors(response.errors());
201+
collectDeleteErrors(response.errors(), errors);
200202
}
203+
errors.log(LOGGER, objectKeys.size(), deleted.size());
201204
return deleted;
202205
}
203206

204-
/**
205-
* Logs per-key delete errors, distinguishing throttling (expected under load, aggregated) from
206-
* hard errors (logged individually). No retry happens here: keys that were not deleted stay marked
207-
* for deletion and are retried on the next FileCleaner cycle, while request-rate backoff is left to
208-
* the S3 client's adaptive retry strategy.
209-
*/
210-
private void logDeleteErrors(final List<S3Error> errors) {
211-
int throttled = 0;
207+
private void collectDeleteErrors(final List<S3Error> errors, final DeleteErrorSummary summary) {
212208
for (final var error : errors) {
213-
if (THROTTLE_ERROR_CODES.contains(error.code())) {
214-
throttled++;
215-
} else {
216-
LOGGER.warn("Failed to delete {}: {} ({}); leaving it for the next cycle",
217-
error.key(), error.message(), error.code());
218-
}
219-
}
220-
if (throttled > 0) {
221-
LOGGER.info("{} keys throttled by S3; leaving them for the next cycle", throttled);
209+
summary.record(
210+
error.code(),
211+
String.format("%s: %s (%s)", error.key(), error.message(), error.code()),
212+
THROTTLE_ERROR_CODES.contains(error.code())
213+
);
222214
}
223215
}
224216

‎storage/inkless/src/test/java/io/aiven/inkless/storage_backend/common/DeleteErrorSummaryTest.java‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/*
2+
* Inkless
3+
* Copyright (C) 2024 - 2026 Aiven OY
4+
*
5+
* This program is free software: you can redistribute it and/or modify
6+
* it under the terms of the GNU Affero General Public License as published by
7+
* the Free Software Foundation, either version 3 of the License, or
8+
* (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU Affero General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU Affero General Public License
16+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
17+
*/
18+
package io.aiven.inkless.storage_backend.common;
19+
20+
import org.junit.jupiter.api.Test;
21+
22+
import java.util.stream.IntStream;
23+
24+
import static org.assertj.core.api.Assertions.assertThat;
25+
26+
/**
27+
* A delete pass can fail for every key it submitted (e.g. a bucket policy denial) and repeats every
28+
* cleanup cycle, so what gets logged must stay bounded by the distinct error codes, not by key count.
29+
*/
30+
class DeleteErrorSummaryTest {
31+
32+
private final DeleteErrorSummary summary = new DeleteErrorSummary();
33+
34+
@Test
35+
void emptyUntilAFailureIsRecorded() {
36+
assertThat(summary.isEmpty()).isTrue();
37+
}
38+
39+
@Test
40+
void samplesAreCappedRegardlessOfKeyCount() {
41+
IntStream.range(0, 1000).forEach(i ->
42+
summary.record("AccessDenied", "key" + i + ": denied", false));
43+
44+
final String formatted = summary.format(1000, 0);
45+
assertThat(formatted)
46+
.startsWith("Failed to delete 1000 of 1000 keys {AccessDenied=1000}")
47+
.contains("key0: denied")
48+
.doesNotContain("key999");
49+
assertThat(formatted.length()).isLessThan(500);
50+
}
51+
52+
@Test
53+
void accumulatesAcrossBatchesAndBreaksDownByCode() {
54+
summary.record("SlowDown", "k0: slow down", true);
55+
summary.record("AccessDenied", "k1: denied", false);
56+
summary.record("SlowDown", "k2: slow down", true);
57+
58+
assertThat(summary.format(4, 1))
59+
.isEqualTo("Failed to delete 3 of 4 keys {AccessDenied=1, SlowDown=2}; e.g. k1: denied");
60+
}
61+
62+
@Test
63+
void throttledFailuresAreCountedButNotSampled() {
64+
summary.record("SlowDown", "k0: slow down", true);
65+
summary.record("ServerBusy", "k1: busy", true);
66+
67+
assertThat(summary.format(2, 0))
68+
.isEqualTo("Failed to delete 2 of 2 keys {ServerBusy=1, SlowDown=1}");
69+
assertThat(summary.firstHardFailure()).isEmpty();
70+
}
71+
72+
@Test
73+
void retainsOnlyTheFirstHardFailureCause() {
74+
final Throwable first = new RuntimeException("first");
75+
summary.record("SlowDown", "k0: slow down", true, new RuntimeException("throttle"));
76+
summary.record("AccessDenied", "k1: denied", false, first);
77+
summary.record("AccessDenied", "k2: denied", false, new RuntimeException("second"));
78+
79+
assertThat(summary.firstHardFailure()).containsSame(first);
80+
}
81+
}

0 commit comments

Comments
 (0)