> On April 16, 2015, 5:34 a.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/MetadataBookkeeper.java, > > line 25 > > <https://reviews.apache.org/r/32937/diff/3/?file=931249#file931249line25> > > > > 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.
I can add some documentation soon. > On April 16, 2015, 5:34 a.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/MetadataBookkeeper.java, > > line 27 > > <https://reviews.apache.org/r/32937/diff/3/?file=931249#file931249line27> > > > > 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. It does not seem like it does currently. I tend towards locks because I like the explicitness, accessibility to source, privateness, and distinction between moniter and condition. But if synchronized in preferred by the project, I can switch it to that. The only piece of functionality the lock would provide, would be if we wanted a timeout on the acquire in awaitUpdate. > On April 16, 2015, 5:34 a.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/MetadataBookkeeper.java, > > line 69 > > <https://reviews.apache.org/r/32937/diff/3/?file=931249#file931249line69> > > > > 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? Reasonable question on the performance. I'll take a look and if there is not much of a difference, move them all to .sets(). As to correctness, my understanding is that since the writes happen in the lock, releasing the lock ensures visability. - Tim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32937/#review80298 ----------------------------------------------------------- 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 > >
