HDDS-14224. Support Range header in HeadObject #9540
Conversation
rich7420
changed the title
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @rich7420 for the patch. Mostly LGTM, few minor code improvements suggested.
| OzoneOutputStream out = bucket.createKey("key1", | ||
| value.getBytes(UTF_8).length, | ||
| ReplicationConfig.fromTypeAndFactor(ReplicationType.RATIS, | ||
| ReplicationFactor.ONE), new HashMap<>()); | ||
| out.write(value.getBytes(UTF_8)); | ||
| out.close(); |
There was a problem hiding this comment.
Please move key creation to setup() to avoid duplication. While at that, please change to:
try (OutputStream out = ...) {
out.write(...)
}| S3GAction s3GAction = S3GAction.HEAD_KEY; | ||
|
|
||
| OzoneKey key; | ||
| RangeHeader rangeHeader = null; |
There was a problem hiding this comment.
Please move to the code block where header is parsed.
| assertEquals("1", response.getHeaderString("Content-Length")); | ||
| assertEquals(String.format("bytes 0-0/%d", value.length()), | ||
| response.getHeaderString("Content-Range")); | ||
| assertEquals("bytes", response.getHeaderString("Accept-Ranges")); |
There was a problem hiding this comment.
Please use constants from HttpHeaders for "Content-Length", etc..
| keyEndpoint = EndpointBuilder.newObjectEndpointBuilder() | ||
| .setClient(client) | ||
| .setHeaders(headers) | ||
| .build(); |
There was a problem hiding this comment.
Please create and set headers on keyEndpoint in setup() and do not create new endpoint in test cases. This also makes it unnecessary to create field for OzoneClient.
| assertEquals(String.format("bytes 0-%d/%d", value.length() - 1, value.length()), | ||
| response.getHeaderString("Content-Range")); | ||
|
|
||
| bucket.deleteKey("key1"); |
There was a problem hiding this comment.
Please move to @AfterEach method to ensure cleanup even in case of test failure.
| .build(); | ||
|
|
||
| //WHEN | ||
| Response response = keyEndpoint.head(bucketName, "key1"); |
There was a problem hiding this comment.
Please define constant for "key1" and reuse in creation, lookup and cleanup.
| OS3Exception ex = assertThrows(OS3Exception.class, | ||
| () -> keyEndpoint.head(bucketName, "key1")); | ||
| assertEquals("InvalidRange", ex.getCode()); | ||
| assertEquals(416, ex.getHttpCode()); |
There was a problem hiding this comment.
Can simplify using EndpointTestUtils:
assertErrorResponse(S3ErrorTable.INVALID_RANGE, () -> keyEndpoint.head(bucketName, "key1"));3 participants
What changes were proposed in this pull request?
In this PR, HEAD Object now handles Range requests just like GET Object does. When you send a Range header, it returns 206 Partial Content with the Content-Range header instead of always returning the full file size.
What is the link to the Apache JIRA
HDDS-14224
How was this patch tested?
https://github.com/rich7420/ozone/actions/runs/20406661096