kfaraz commented on code in PR #16481:
URL: https://github.com/apache/druid/pull/16481#discussion_r1627919294
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/RetryableS3OutputStream.java:
##########
@@ -269,52 +213,74 @@ public void close() throws IOException
// Closeables are closed in LIFO order
closer.register(() -> {
// This should be emitted as a metric
+ long totalChunkSize = (currentChunk.id - 1) * chunkSize +
currentChunk.length();
Review Comment:
> It allows us to not have a Map<Chunk, Future> unnecessarily
To me, having the map seems cleaner as we know which future belongs to which
chunk. I don't see how it would affect the rest of the code as we just need to
put stuff in the map and then iterate over it in `completeMultipartUpload()`
and then while determining the size (which could even just be returned by the
`completeMultipartUpload()` method).
That said, I don't really mind the current approach either. I guess the map
is more of a personal preference in this case.
--
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: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
