LakshSingla commented on code in PR #16481: URL: https://github.com/apache/druid/pull/16481#discussion_r1616633265
########## extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3UploadConfig.java: ########## @@ -0,0 +1,115 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.storage.s3.output; + +import org.apache.druid.guice.LazySingleton; +import org.apache.druid.java.util.common.HumanReadableBytes; +import org.apache.druid.java.util.common.logger.Logger; + +import java.util.concurrent.atomic.AtomicInteger; + +/** + * This class manages the configuration for uploading files to S3 in chunks. + * It tracks the current number of chunks written to local disk concurrently and ensures that the + * maximum number of chunks on disk does not exceed a specified limit at any given point in time. + */ +@LazySingleton +public class S3UploadConfig +{ + /** + * The maximum chunk size based on injected values for {@link S3OutputConfig} and {@link S3ExportConfig} used for computing maximum number of chunks to save on disk at any given point in time. + * It is initialized to 5 MiB which is the minimum chunk size possible, denoted by {@link S3OutputConfig#S3_MULTIPART_UPLOAD_MIN_PART_SIZE_BYTES}. + */ + private long chunkSize = new HumanReadableBytes("5MiB").getBytes(); + + /** + * An atomic counter to track the current number of chunks saved to local disk. + */ + private final AtomicInteger currentNumChunks = new AtomicInteger(0); + + /** + * The maximum number of chunks that can be saved to local disk concurrently. + * This value is recalculated when the chunk size is updated in {@link #updateChunkSizeIfGreater(long)}. + */ + private int maxConcurrentNumChunks = 100; Review Comment: As a reader, this isn't clear at all. The class works for the current use case, however it isn't thread-safe. This is pretty error-prone if multiple threads decide to call `updateChunkSizeIfGreater` later, and we are expecting this class to be called with multiple threads. I'd either make it thread-safe throughout or make it thread-unsafe and expect the callers to do the synchronization. -- 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]
