----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32937/#review80298 -----------------------------------------------------------
This version looks a lot better than the original patch -- it's a lot clearer what's going on and some of the error-prone tricks have been removed. I also like the separation of the management of the Cluster object from the request/versioning stuff as long as we can convince ourselves that it's safe to do so, I also found the combination confusing to reason about. I filed a couple of issues. I'd probably also want to double check the last update/timeout code -- it was very difficult to get that working properly under all conditions, so I'd want to be certain we didn't lose anything in moving that around and changing how successes/failures are handled. clients/src/main/java/org/apache/kafka/clients/MetadataBookkeeper.java <https://reviews.apache.org/r/32937/#comment130108> This patch will definitely need a comment somewhere explaining the locking strategy and the reasoning behind it. It's won't be obvious even to someone familiar with other client code why this all works the way it does. clients/src/main/java/org/apache/kafka/clients/MetadataBookkeeper.java <https://reviews.apache.org/r/32937/#comment130115> Is there any benefit to using a Lock + Condition instead of the monitor for the object? Seems like it would make the code a bit simpler - you'd get rid of all the try/finally blocks. clients/src/main/java/org/apache/kafka/clients/MetadataBookkeeper.java <https://reviews.apache.org/r/32937/#comment130111> Are the lazySets accomplishing anything performance-wise? I doubt most people even know the method exists, let alone the implications. Some seem like they're probably ok, e.g. none of the code that uses the version number relies on seeing the updated value immediately. But others I'm less sure about (and the lack of documentation on lazySet makes it hard to be certain). For example, using updateRequested.lazySet() might work, but could give incorrect results if the old value is returned to a different thread. I think this only affects the network thread in the producer, but the consumer can potentially be called from different threads. Are we sure the lazySet works as expected in that case? - Ewen Cheslack-Postava On April 16, 2015, 12:56 a.m., Tim Brooks wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32937/ > ----------------------------------------------------------- > > (Updated April 16, 2015, 12:56 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2102 > https://issues.apache.org/jira/browse/KAFKA-2102 > > > Repository: kafka > > > Description > ------- > > Method does not need to be synchronized > > > Do not synchronize contains topic method > > > Continue removing the need to synchronize the metadata object > > > Store both last refresh and need to refresh in same variable > > > Fix synchronize issue > > > Version needs to be volatile > > > rework how signally happens > > > remove unnecessary creation of new set > > > initialize 0 at the field level > > > Fix the build > > > Start moving synchronization of metadata to different class > > > Start moving synchronization work to new class > > > Remove unused code > > > Functionality works. Not threadsafe > > > move version into metadata synchronizer > > > Make version volatile > > > Rename classes > > > move to finergrained locking > > > Use locks in bookkeeper > > > Only use atomic variabled > > > use successful metadata in metrics > > > Change these things back to trunk > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/Metadata.java > 07f1cdb1fe920b0c7a5f2d101ddc40c689e1b247 > clients/src/main/java/org/apache/kafka/clients/MetadataBookkeeper.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java > b7ae595f2cc46e5dfe728bc3ce6082e9cd0b6d36 > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > b91e2c52ed0acb1faa85915097d97bafa28c413a > > Diff: https://reviews.apache.org/r/32937/diff/ > > > Testing > ------- > > > Thanks, > > Tim Brooks > >
