adutra commented on code in PR #15428:
URL: https://github.com/apache/iceberg/pull/15428#discussion_r2895212304
##########
aws/src/integration/java/org/apache/iceberg/aws/s3/signer/TestS3RestSigner.java:
##########
@@ -202,24 +209,65 @@ private static Server initHttpServer() throws Exception {
return server;
}
+ /**
+ * Assert the cache hits and misses match the values.
+ *
+ * @param hits expected hits
+ * @param misses expected misses
+ */
+ private void assertCacheHitsAndMisses(int hits, int misses) {
+ assertThat(S3V4RestSignerClient.cacheHits()).describedAs("Cache
hits").isEqualTo(hits);
+ assertThat(S3V4RestSignerClient.cacheMisses()).describedAs("Cache
misses").isEqualTo(misses);
+ }
+
@Test
public void validateGetObject() {
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").build());
- // signer caching should kick in when repeating the same request
-
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").build());
+ assertCacheHitsAndMisses(0, 1);
+
+ // signer caching should kick in when repeating the same request with a
range
+
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").range("0-10").build());
+ assertCacheHitsAndMisses(1, 1);
+ }
+
+ @Test
+ public void validateHeadObjectUnsignedHeaders() {
+ final HeadObjectResponse response =
+
s3.headObject(HeadObjectRequest.builder().bucket(BUCKET).key("random/key").build());
+ assertCacheHitsAndMisses(0, 1);
+
+ // the etag is passed in: the same object is returned and the same cached
signature is retained.
+ // if the ifMatch header was cached, this would have resulted in a failure
as there would
+ // be a signature mismatch.
+ s3.headObject(
+ HeadObjectRequest.builder()
+ .bucket(BUCKET)
+ .key("random/key")
+ .ifMatch(response.eTag())
+ .build());
+ assertCacheHitsAndMisses(1, 1);
}
@Test
public void validatePutObject() {
+ int hits = S3V4RestSignerClient.cacheHits();
Review Comment:
```suggestion
```
##########
aws/src/integration/java/org/apache/iceberg/aws/s3/signer/TestS3RestSigner.java:
##########
@@ -202,24 +209,65 @@ private static Server initHttpServer() throws Exception {
return server;
}
+ /**
+ * Assert the cache hits and misses match the values.
+ *
+ * @param hits expected hits
+ * @param misses expected misses
+ */
+ private void assertCacheHitsAndMisses(int hits, int misses) {
+ assertThat(S3V4RestSignerClient.cacheHits()).describedAs("Cache
hits").isEqualTo(hits);
+ assertThat(S3V4RestSignerClient.cacheMisses()).describedAs("Cache
misses").isEqualTo(misses);
+ }
+
@Test
public void validateGetObject() {
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").build());
- // signer caching should kick in when repeating the same request
-
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").build());
+ assertCacheHitsAndMisses(0, 1);
+
+ // signer caching should kick in when repeating the same request with a
range
+
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").range("0-10").build());
+ assertCacheHitsAndMisses(1, 1);
+ }
+
+ @Test
+ public void validateHeadObjectUnsignedHeaders() {
+ final HeadObjectResponse response =
+
s3.headObject(HeadObjectRequest.builder().bucket(BUCKET).key("random/key").build());
+ assertCacheHitsAndMisses(0, 1);
+
+ // the etag is passed in: the same object is returned and the same cached
signature is retained.
+ // if the ifMatch header was cached, this would have resulted in a failure
as there would
+ // be a signature mismatch.
+ s3.headObject(
+ HeadObjectRequest.builder()
+ .bucket(BUCKET)
+ .key("random/key")
+ .ifMatch(response.eTag())
+ .build());
+ assertCacheHitsAndMisses(1, 1);
}
@Test
public void validatePutObject() {
+ int hits = S3V4RestSignerClient.cacheHits();
s3.putObject(
PutObjectRequest.builder().bucket(BUCKET).key("some/key").build(),
Paths.get("/etc/hosts"));
+ assertCacheHitsAndMisses(0, 1);
+ s3.putObject(
+ PutObjectRequest.builder().bucket(BUCKET).key("some/key").build(),
+ RequestBody.fromString("update"));
+ assertCacheHitsAndMisses(0, 2);
}
@Test
public void validateDeleteObjects() {
+ int hits = S3V4RestSignerClient.cacheHits();
+ int misses = S3V4RestSignerClient.cacheMisses();
Review Comment:
```suggestion
```
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -72,11 +78,57 @@ public abstract class S3V4RestSignerClient
static final String CACHE_CONTROL_PRIVATE = "private";
static final String CACHE_CONTROL_NO_CACHE = "no-cache";
- private static final Cache<Key, SignedComponent> SIGNED_COMPONENT_CACHE =
+ @VisibleForTesting
+ static final Cache<S3SignRequest, SignedComponent> SIGNED_COMPONENT_CACHE =
Caffeine.newBuilder().expireAfterWrite(30,
TimeUnit.SECONDS).maximumSize(100).build();
+ private static final AtomicInteger CACHE_HITS = new AtomicInteger();
Review Comment:
Can't we use Caffeine's `.recordStats()` instead? And ideally, we would call
`.recordStats()` only when running tests.
##########
aws/src/integration/java/org/apache/iceberg/aws/s3/signer/TestS3RestSigner.java:
##########
@@ -228,19 +276,36 @@ public void validateDeleteObjects() {
ObjectIdentifier.builder().key("some/key2").build())
.build();
-
s3.deleteObjects(DeleteObjectsRequest.builder().bucket(BUCKET).delete(objectsToDelete).build());
+ final DeleteObjectsRequest request =
+
DeleteObjectsRequest.builder().bucket(BUCKET).delete(objectsToDelete).build();
+ s3.deleteObjects(request);
+ assertCacheHitsAndMisses(0, 1);
+
+ // issue exactly the same object. DELETE must never be cached as all paths
+ // need review by the remote service.
+ s3.deleteObjects(request);
+ assertCacheHitsAndMisses(0, 2);
}
@Test
public void validateListPrefix() {
s3.listObjectsV2(ListObjectsV2Request.builder().bucket(BUCKET).prefix("some/prefix/").build());
+ assertCacheHitsAndMisses(0, 1);
+
s3.listObjectsV2(ListObjectsV2Request.builder().bucket(BUCKET).prefix("some/prefix/").build());
+ // list is a GET.
+ assertCacheHitsAndMisses(1, 1);
+ assertCacheHitsAndMisses(1, 1);
Review Comment:
```suggestion
```
##########
aws/src/integration/java/org/apache/iceberg/aws/s3/signer/TestS3RestSigner.java:
##########
@@ -202,24 +209,65 @@ private static Server initHttpServer() throws Exception {
return server;
}
+ /**
+ * Assert the cache hits and misses match the values.
+ *
+ * @param hits expected hits
+ * @param misses expected misses
+ */
+ private void assertCacheHitsAndMisses(int hits, int misses) {
+ assertThat(S3V4RestSignerClient.cacheHits()).describedAs("Cache
hits").isEqualTo(hits);
+ assertThat(S3V4RestSignerClient.cacheMisses()).describedAs("Cache
misses").isEqualTo(misses);
+ }
+
@Test
public void validateGetObject() {
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").build());
- // signer caching should kick in when repeating the same request
-
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").build());
+ assertCacheHitsAndMisses(0, 1);
+
+ // signer caching should kick in when repeating the same request with a
range
+
s3.getObject(GetObjectRequest.builder().bucket(BUCKET).key("random/key").range("0-10").build());
+ assertCacheHitsAndMisses(1, 1);
+ }
+
+ @Test
+ public void validateHeadObjectUnsignedHeaders() {
+ final HeadObjectResponse response =
+
s3.headObject(HeadObjectRequest.builder().bucket(BUCKET).key("random/key").build());
+ assertCacheHitsAndMisses(0, 1);
+
+ // the etag is passed in: the same object is returned and the same cached
signature is retained.
+ // if the ifMatch header was cached, this would have resulted in a failure
as there would
Review Comment:
```suggestion
// if the ifMatch header was signed, this would have resulted in a
failure as there would
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
