peterxcli commented on code in PR #11136:
URL: https://github.com/apache/ozone/pull/11136#discussion_r3879565347
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1115,8 +1121,9 @@ srcKeyLen > getDatastreamMinLength()) {
getMetrics().updateCopyKeyMetadataStats(startNanos);
perf.appendMetaLatencyNanos(metadataLatencyNs);
copyLength = dest.copyFrom(src, getIOBufferSize(expectedLength));
- String md5Hash =
DatatypeConverter.printHexBinary(src.getMessageDigest().digest()).toLowerCase();
- dest.getMetadata().put(OzoneConsts.ETAG, md5Hash);
+ String eTag = reusedETag != null ? reusedETag
+ : DatatypeConverter.printHexBinary(((DigestInputStream)
src).getMessageDigest().digest()).toLowerCase();
+ dest.getMetadata().put(OzoneConsts.ETAG, eTag);
Review Comment:
Good catch, fixed in 81061dd.
Rather than add a runtime contract check, I went the other way and made the
unsafe combination unrepresentable: `copy(...)` keeps its original
`DigestInputStream` parameter type, and the reuse path switches hashing off
with `DigestInputStream#on(false)` instead of passing a bare `InputStream`. So
there is no cast left, and the implicit "`src` is a `DigestInputStream` iff
`reusedETag == null`" coupling between two parameters is gone.
Side benefit: the only signature change against master is now the added
`reusedETag` parameter, so the diff is smaller than before.
`TestObjectPut#testCopyObjectReusesSourceETagWithoutRehashing` already stubs
the thread-local MD5 digest to throw if `update` is called, so it also pins the
`on(false)` behaviour. Re-benchmarked to confirm the win survives: 64 MiB
CopyObject 104.35 ms -> 10.32 ms (90.1%), and a counting `MessageDigest`
confirms the reuse path still feeds 0 bytes to MD5 while the digesting path
feeds the full 67,108,864.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpointStreaming.java:
##########
@@ -189,8 +196,8 @@ public static long copyKeyWithStream(
long metadataLatencyNs =
METRICS.updateCopyKeyMetadataStats(startNanos);
writeLen = writeGuard.copyFrom(body, bufferSize);
- String eTag =
DatatypeConverter.printHexBinary(body.getMessageDigest().digest())
- .toLowerCase();
+ String eTag = reusedETag != null ? reusedETag
+ : DatatypeConverter.printHexBinary(((DigestInputStream)
body).getMessageDigest().digest()).toLowerCase();
perf.appendMetaLatencyNanos(metadataLatencyNs);
writeGuard.getMetadata().put(OzoneConsts.ETAG, eTag);
Review Comment:
Fixed in 81061dd, same approach as the sibling comment on `ObjectEndpoint`.
I took the second of your two suggestions: `copyKeyWithStream(...)` keeps
`body` typed as `DigestInputStream`, and the caller switches hashing off with
`DigestInputStream#on(false)` when it is going to reuse the source ETag. No
cast, no runtime check needed, and both write paths now express the same thing
the same way.
Verified on the datastream path specifically by
`TestUploadWithStream#testUploadWithCopyReusesSourceETag` (asserts the stored
ETag is the reused one, not a recomputed MD5) and
`#testUploadWithCopyRecomputesETagForMultipartSource` (asserts a `-N` source
still gets a fresh content MD5).
--
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]
