LakshSingla commented on code in PR #16481: URL: https://github.com/apache/druid/pull/16481#discussion_r1617126183
########## extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/output/S3UploadManager.java: ########## Review Comment: Note on the synchronization of the class - it is still not thread safe. Consider two threads T1 & T2 calling `updateChunkSizeIfGreater(100)` and `updateChunkSizeIfGreater(200)` respectively, and the current `chunkSize` is 50. Both can simultaneously enter the method and at the end of the two operations, the `chunkSize` can be 100 instead of 200 (which was expected). Making the integers atomic isn't enough since there are conditions in which multiple variables are getting updated/read. The easiest way of synchronizing it would be to leave all the variables as is (and not Atomic variables), and mark all the methods `synchronized`. -- 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]
