kfaraz commented on code in PR #16481:
URL: https://github.com/apache/druid/pull/16481#discussion_r1609573706
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/RetryableS3OutputStream.java:
##########
@@ -103,22 +107,32 @@ public class RetryableS3OutputStream extends OutputStream
private boolean error;
private boolean closed;
- public RetryableS3OutputStream(
- S3OutputConfig config,
- ServerSideEncryptingAmazonS3 s3,
- String s3Key
- ) throws IOException
- {
+ /**
+ * An atomic counter to store number of files pending to be uploaded for the
particular uploadId.
+ */
+ private final AtomicInteger pendingFiles = new AtomicInteger(0);
- this(config, s3, s3Key, true);
- }
+ /**
+ * A lock used for notifying the main thread about the completion of
s3.uploadPart() for all chunks
+ * and hence starting the s3.completeMultipartUpload() for the uploadId.
+ */
+ private final Object fileLock = new Object();
+
+ /**
+ * Semaphore to restrict the maximum number of simultaneous chunks on disk.
+ */
+ private static final int MAX_CONCURRENT_CHUNKS = 10;
+ private static final Semaphore SEMAPHORE = new
Semaphore(MAX_CONCURRENT_CHUNKS);
- @VisibleForTesting
- protected RetryableS3OutputStream(
+ /**
+ * Threadpool used for uploading the chunks asynchronously.
+ */
+ private static final ExecutorService UPLOAD_EXECUTOR =
Execs.multiThreaded(10, "UploadThreadPool-%d");
Review Comment:
> Seems unnecessary to me? For my understanding, what benefit does it
provide over the current approach?
It is not unnecessary, it is the right approach 🙂 . `static` should be used
for constants, static variables and static classes. It doesn't make sense to
make executors and other resources as static for the sole purpose of sharing
them across instances. It makes the code difficult to follow and interferes
with the lifecycle of such resources. You never know when to clean up such a
resource as you don't know if an instance still exists in the JVM.
An executor is a resource and if it is meant to be shared, it must be
explicitly passed into the constructor as a dependency.
--
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]
