----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33731/#review83694 -----------------------------------------------------------
Thanks for the patch. A few comments. 1. The changes in Pool are pretty complicated. It's not clear to me if the changes are general purpose. Will it be better to implement a simpler version just for the purgatory? 2. I am not sure if all concurrent updates are handled properly. Some of the complexity is that keys can be removed using remove() as well as updateAndMaybeRemove and they all need to be synchronized properly with the logic of adding keys. See the comments below, 3. Could you explain the use cases in coordinator a bit more? Given the complexity added to purgatory, I am wondering if it's better to have a special implementation just for coordinator. core/src/main/scala/kafka/server/DelayedOperation.scala <https://reviews.apache.org/r/33731/#comment134743> In this case, if the key doesn't exist, we will be adding an entry to the map and then remove it? That will add unnecessary overhead. For example, every replica fetch request will now have this overhead when checking the produce purgatory, which could be significant. core/src/main/scala/kafka/utils/Pool.scala <https://reviews.apache.org/r/33731/#comment134736> associate => associated core/src/main/scala/kafka/utils/Pool.scala <https://reviews.apache.org/r/33731/#comment134744> It seems that the key can be removed through remove() after line 119. Then, it can happen that an item that should be watched is actually not being watched? - Jun Rao On May 6, 2015, 11:31 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33731/ > ----------------------------------------------------------- > > (Updated May 6, 2015, 11:31 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2160 > https://issues.apache.org/jira/browse/KAFKA-2160 > > > Repository: kafka > > > Description > ------- > > Add a per-key lock in Pool which is used for non-primitive > UpdateAndMaybeRemove function > > > Diffs > ----- > > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala > aa8d9404a3e78a365df06404b79d0d8f694b4bd6 > core/src/main/scala/kafka/server/DelayedOperation.scala > 2ed9b467c2865e5717d7f6fd933cd09a5c5b22c0 > core/src/main/scala/kafka/server/DelayedProduce.scala > 05078b24ef28f2f4e099afa943e43f1d00359fda > core/src/main/scala/kafka/utils/Pool.scala > 9ddcde797341ddd961923cafca16472d84417b5a > core/src/main/scala/kafka/utils/timer/Timer.scala > b8cde820a770a4e894804f1c268b24b529940650 > core/src/test/scala/unit/kafka/log/LogConfigTest.scala > f3546adee490891e0d8d0214bef00b1dd7f42227 > core/src/test/scala/unit/kafka/server/DelayedOperationTest.scala > f3ab3f4ff8eb1aa6b2ab87ba75f72eceb6649620 > > Diff: https://reviews.apache.org/r/33731/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
