tyamashi-oss commented on code in PR #12296: URL: https://github.com/apache/kafka/pull/12296#discussion_r913698340
########## core/src/main/scala/kafka/utils/Throttler.scala: ########## @@ -36,7 +36,7 @@ import scala.math._ * @param time: The time implementation to use */ @threadsafe -class Throttler(desiredRatePerSec: Double, +class Throttler(var desiredRatePerSec: Double, Review Comment: Thanks for pointing out, @showuon and @tombentley. Indeed, it needs volatile or synchronized. I would like your opinion. Should it use volatile? or synchronized? I have changed the implementation once with volatile, so if you prefer synchronized, please let me know. https://github.com/apache/kafka/pull/12296/commits/9b2477dc8a38db7e41281a6c5aba9c19348e8d5d If use synchronized, do we apply a synchronized block to this object as following code? I think `lock` object cannot be used for synchronized in updateDesiredRatePerSec() like `lock synchronized {…} block in maybeThrottle()`, it may take more than a few minute due to frequent sleeps depending on Throttler.desiredRatePerSec, and may block reconfigure() thread at updateDesiredRatePerSec() as well. ``` def updateDesiredRatePerSec(updatedDesiredRatePerSec: Double): Unit = { this synchronized { desiredRatePerSec = updatedDesiredRatePerSec; } } ``` -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
