Skip to content

Commit aec6f6d

Browse files
KodaiDKodaiD
committed
Apply suggestions
1 parent 001d495 commit aec6f6d

File tree

2 files changed

+113
-67
lines changed

2 files changed

+113
-67
lines changed

‎core/src/main/java/com/scalar/db/storage/objectstorage/s3/S3Wrapper.java‎

Lines changed: 88 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,9 @@ public Optional<ObjectStorageWrapperResponse> get(String key)
8080
String eTag = response.response().eTag();
8181
return Optional.of(new ObjectStorageWrapperResponse(data, eTag));
8282
} catch (Exception e) {
83-
Throwable cause = e.getCause();
84-
if (cause instanceof S3Exception) {
85-
S3Exception s3Exception = (S3Exception) cause;
86-
if (s3Exception.statusCode() == S3ErrorCode.NOT_FOUND.get()) {
83+
Optional<S3Exception> s3Exception = findS3Exception(e);
84+
if (s3Exception.isPresent()) {
85+
if (s3Exception.get().statusCode() == S3ErrorCode.NOT_FOUND.get()) {
8786
return Optional.empty();
8887
}
8988
}
@@ -121,17 +120,19 @@ public void insert(String key, String object) throws ObjectStorageWrapperExcepti
121120
} catch (Exception e) {
122121
Throwable cause = e.getCause();
123122
if (cause instanceof S3Exception) {
124-
S3Exception s3Exception = (S3Exception) cause;
125-
if (s3Exception.statusCode() == S3ErrorCode.CONFLICT.get()) {
126-
throw new ConflictOccurredException(
127-
String.format("Failed to insert the object with key '%s' due to conflict", key),
128-
s3Exception);
129-
}
130-
if (s3Exception.statusCode() == S3ErrorCode.PRECONDITION_FAILED.get()) {
131-
throw new PreconditionFailedException(
132-
String.format(
133-
"Failed to insert the object with key '%s' due to precondition failure", key),
134-
s3Exception);
123+
Optional<S3Exception> s3Exception = findS3Exception(e);
124+
if (s3Exception.isPresent()) {
125+
if (s3Exception.get().statusCode() == S3ErrorCode.CONFLICT.get()) {
126+
throw new ConflictOccurredException(
127+
String.format("Failed to insert the object with key '%s' due to conflict", key),
128+
s3Exception.get());
129+
}
130+
if (s3Exception.get().statusCode() == S3ErrorCode.PRECONDITION_FAILED.get()) {
131+
throw new PreconditionFailedException(
132+
String.format(
133+
"Failed to insert the object with key '%s' due to precondition failure", key),
134+
s3Exception.get());
135+
}
135136
}
136137
}
137138
throw new ObjectStorageWrapperException(
@@ -151,14 +152,20 @@ public void update(String key, String object, String version)
151152
} catch (Exception e) {
152153
Throwable cause = e.getCause();
153154
if (cause instanceof S3Exception) {
154-
S3Exception s3Exception = (S3Exception) cause;
155-
if (s3Exception.statusCode() == S3ErrorCode.NOT_FOUND.get()
156-
|| s3Exception.statusCode() == S3ErrorCode.CONFLICT.get()
157-
|| s3Exception.statusCode() == S3ErrorCode.PRECONDITION_FAILED.get()) {
158-
throw new PreconditionFailedException(
159-
String.format(
160-
"Failed to update the object with key '%s' due to precondition failure", key),
161-
s3Exception);
155+
Optional<S3Exception> s3Exception = findS3Exception(e);
156+
if (s3Exception.isPresent()) {
157+
if (s3Exception.get().statusCode() == S3ErrorCode.CONFLICT.get()) {
158+
throw new ConflictOccurredException(
159+
String.format("Failed to update the object with key '%s' due to conflict", key),
160+
s3Exception.get());
161+
}
162+
if (s3Exception.get().statusCode() == S3ErrorCode.NOT_FOUND.get()
163+
|| s3Exception.get().statusCode() == S3ErrorCode.PRECONDITION_FAILED.get()) {
164+
throw new PreconditionFailedException(
165+
String.format(
166+
"Failed to update the object with key '%s' due to precondition failure", key),
167+
s3Exception.get());
168+
}
162169
}
163170
}
164171
throw new ObjectStorageWrapperException(
@@ -175,12 +182,19 @@ public void delete(String key) throws ObjectStorageWrapperException {
175182
} catch (Exception e) {
176183
Throwable cause = e.getCause();
177184
if (cause instanceof S3Exception) {
178-
S3Exception s3Exception = (S3Exception) cause;
179-
if (s3Exception.statusCode() == S3ErrorCode.NOT_FOUND.get()) {
180-
throw new PreconditionFailedException(
181-
String.format(
182-
"Failed to delete the object with key '%s' due to precondition failure", key),
183-
s3Exception);
185+
Optional<S3Exception> s3Exception = findS3Exception(e);
186+
if (s3Exception.isPresent()) {
187+
if (s3Exception.get().statusCode() == S3ErrorCode.CONFLICT.get()) {
188+
throw new ConflictOccurredException(
189+
String.format("Failed to delete the object with key '%s' due to conflict", key),
190+
s3Exception.get());
191+
}
192+
if (s3Exception.get().statusCode() == S3ErrorCode.NOT_FOUND.get()) {
193+
throw new PreconditionFailedException(
194+
String.format(
195+
"Failed to delete the object with key '%s' due to precondition failure", key),
196+
s3Exception.get());
197+
}
184198
}
185199
}
186200
throw new ObjectStorageWrapperException(
@@ -198,14 +212,20 @@ public void delete(String key, String version) throws ObjectStorageWrapperExcept
198212
} catch (Exception e) {
199213
Throwable cause = e.getCause();
200214
if (cause instanceof S3Exception) {
201-
S3Exception s3Exception = (S3Exception) cause;
202-
if (s3Exception.statusCode() == S3ErrorCode.NOT_FOUND.get()
203-
|| s3Exception.statusCode() == S3ErrorCode.CONFLICT.get()
204-
|| s3Exception.statusCode() == S3ErrorCode.PRECONDITION_FAILED.get()) {
205-
throw new PreconditionFailedException(
206-
String.format(
207-
"Failed to delete the object with key '%s' due to precondition failure", key),
208-
s3Exception);
215+
Optional<S3Exception> s3Exception = findS3Exception(e);
216+
if (s3Exception.isPresent()) {
217+
if (s3Exception.get().statusCode() == S3ErrorCode.CONFLICT.get()) {
218+
throw new ConflictOccurredException(
219+
String.format("Failed to delete the object with key '%s' due to conflict", key),
220+
s3Exception.get());
221+
}
222+
if (s3Exception.get().statusCode() == S3ErrorCode.NOT_FOUND.get()
223+
|| s3Exception.get().statusCode() == S3ErrorCode.PRECONDITION_FAILED.get()) {
224+
throw new PreconditionFailedException(
225+
String.format(
226+
"Failed to delete the object with key '%s' due to precondition failure", key),
227+
s3Exception.get());
228+
}
209229
}
210230
}
211231
throw new ObjectStorageWrapperException(
@@ -216,35 +236,35 @@ public void delete(String key, String version) throws ObjectStorageWrapperExcept
216236
@Override
217237
public void deleteByPrefix(String prefix) throws ObjectStorageWrapperException {
218238
try {
219-
List<ObjectIdentifier> identifiers = new ArrayList<>();
220-
221239
client
222240
.listObjectsV2Paginator(
223241
ListObjectsV2Request.builder().bucket(bucket).prefix(prefix).build())
224242
.subscribe(
225-
response ->
243+
response -> {
244+
if (!response.contents().isEmpty()) {
245+
List<ObjectIdentifier> pageObjects = new ArrayList<>();
246+
// Collect all objects from this page
226247
response
227248
.contents()
228249
.forEach(
229250
s3Object ->
230-
identifiers.add(
231-
ObjectIdentifier.builder().key(s3Object.key()).build())))
251+
pageObjects.add(
252+
ObjectIdentifier.builder().key(s3Object.key()).build()));
253+
// Delete objects from this page in batches
254+
for (int i = 0; i < pageObjects.size(); i += BATCH_DELETE_SIZE_LIMIT) {
255+
int endIndex = Math.min(i + BATCH_DELETE_SIZE_LIMIT, pageObjects.size());
256+
List<ObjectIdentifier> batch = pageObjects.subList(i, endIndex);
257+
client
258+
.deleteObjects(
259+
DeleteObjectsRequest.builder()
260+
.bucket(bucket)
261+
.delete(Delete.builder().objects(batch).build())
262+
.build())
263+
.join();
264+
}
265+
}
266+
})
232267
.join();
233-
234-
if (!identifiers.isEmpty()) {
235-
final int totalSize = identifiers.size();
236-
for (int i = 0; i < totalSize; i += BATCH_DELETE_SIZE_LIMIT) {
237-
int endIndex = Math.min(i + BATCH_DELETE_SIZE_LIMIT, totalSize);
238-
List<ObjectIdentifier> batch = identifiers.subList(i, endIndex);
239-
client
240-
.deleteObjects(
241-
DeleteObjectsRequest.builder()
242-
.bucket(bucket)
243-
.delete(Delete.builder().objects(batch).build())
244-
.build())
245-
.join();
246-
}
247-
}
248268
} catch (Exception e) {
249269
throw new ObjectStorageWrapperException(
250270
String.format("Failed to delete the objects with prefix '%s'", prefix), e);
@@ -255,4 +275,15 @@ public void deleteByPrefix(String prefix) throws ObjectStorageWrapperException {
255275
public void close() {
256276
client.close();
257277
}
278+
279+
private Optional<S3Exception> findS3Exception(Throwable throwable) {
280+
Throwable current = throwable;
281+
while (current != null) {
282+
if (current instanceof S3Exception) {
283+
return Optional.of((S3Exception) current);
284+
}
285+
current = current.getCause();
286+
}
287+
return Optional.empty();
288+
}
258289
}

‎core/src/test/java/com/scalar/db/storage/objectstorage/s3/S3WrapperTest.java‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@
88
import static org.mockito.Mockito.verify;
99
import static org.mockito.Mockito.when;
1010

11+
import com.scalar.db.storage.objectstorage.ConflictOccurredException;
1112
import com.scalar.db.storage.objectstorage.ObjectStorageWrapperException;
1213
import com.scalar.db.storage.objectstorage.ObjectStorageWrapperResponse;
1314
import com.scalar.db.storage.objectstorage.PreconditionFailedException;
1415
import java.util.Arrays;
1516
import java.util.Collections;
17+
import java.util.List;
1618
import java.util.Optional;
1719
import java.util.Set;
1820
import java.util.concurrent.CompletableFuture;
@@ -303,7 +305,7 @@ public void update_S3ExceptionWith412Thrown_ShouldThrowPreconditionFailedExcepti
303305
}
304306

305307
@Test
306-
public void update_S3ExceptionWith409Thrown_ShouldThrowPreconditionFailedException() {
308+
public void update_S3ExceptionWith409Thrown_ShouldThrowConflictOccurredException() {
307309
// Arrange
308310
CompletableFuture<PutObjectResponse> failedFuture = new CompletableFuture<>();
309311
failedFuture.completeExceptionally(S3Exception.builder().statusCode(409).build());
@@ -312,7 +314,7 @@ public void update_S3ExceptionWith409Thrown_ShouldThrowPreconditionFailedExcepti
312314

313315
// Act & Assert
314316
assertThatCode(() -> wrapper.update(ANY_OBJECT_KEY, ANY_DATA, ANY_ETAG))
315-
.isInstanceOf(PreconditionFailedException.class);
317+
.isInstanceOf(ConflictOccurredException.class);
316318
}
317319

318320
@Test
@@ -413,15 +415,15 @@ public void delete_WithVersion_S3ExceptionWith412Thrown_ShouldThrowPreconditionF
413415
}
414416

415417
@Test
416-
public void delete_WithVersion_S3ExceptionWith409Thrown_ShouldThrowPreconditionFailedException() {
418+
public void delete_WithVersion_S3ExceptionWith409Thrown_ShouldThrowConflictOccurredException() {
417419
// Arrange
418420
CompletableFuture<DeleteObjectResponse> failedFuture = new CompletableFuture<>();
419421
failedFuture.completeExceptionally(S3Exception.builder().statusCode(409).build());
420422
when(client.deleteObject(any(DeleteObjectRequest.class))).thenReturn(failedFuture);
421423

422424
// Act & Assert
423425
assertThatCode(() -> wrapper.delete(ANY_OBJECT_KEY, ANY_ETAG))
424-
.isInstanceOf(PreconditionFailedException.class);
426+
.isInstanceOf(ConflictOccurredException.class);
425427
}
426428

427429
@Test
@@ -481,14 +483,27 @@ public void deleteByPrefix_PrefixGiven_ShouldDeleteAllObjectsWithThePrefix() thr
481483
// Assert
482484
ArgumentCaptor<DeleteObjectsRequest> requestCaptor =
483485
ArgumentCaptor.forClass(DeleteObjectsRequest.class);
484-
verify(client).deleteObjects(requestCaptor.capture());
486+
verify(client, org.mockito.Mockito.times(2)).deleteObjects(requestCaptor.capture());
487+
488+
// Verify that all objects were deleted across the two calls
489+
List<DeleteObjectsRequest> capturedRequests = requestCaptor.getAllValues();
490+
assertThat(capturedRequests).hasSize(2);
491+
492+
// First page: should delete object1 and object2
493+
DeleteObjectsRequest firstRequest = capturedRequests.get(0);
494+
assertThat(firstRequest.bucket()).isEqualTo(BUCKET);
495+
assertThat(firstRequest.delete().objects()).hasSize(2);
496+
assertThat(firstRequest.delete().objects())
497+
.extracting(ObjectIdentifier::key)
498+
.containsExactlyInAnyOrder(objectKey1, objectKey2);
485499

486-
DeleteObjectsRequest capturedRequest = requestCaptor.getValue();
487-
assertThat(capturedRequest.bucket()).isEqualTo(BUCKET);
488-
assertThat(capturedRequest.delete().objects()).hasSize(3);
489-
assertThat(capturedRequest.delete().objects())
500+
// Second page: should delete object3
501+
DeleteObjectsRequest secondRequest = capturedRequests.get(1);
502+
assertThat(secondRequest.bucket()).isEqualTo(BUCKET);
503+
assertThat(secondRequest.delete().objects()).hasSize(1);
504+
assertThat(secondRequest.delete().objects())
490505
.extracting(ObjectIdentifier::key)
491-
.containsExactlyInAnyOrder(objectKey1, objectKey2, objectKey3);
506+
.containsExactly(objectKey3);
492507
}
493508

494509
@Test

0 commit comments

Comments
 (0)