Jay, About 1), I just remembered why I did not use a while loop inside forceUpdate, which is because every time we are interrupted but not jumping out of the loop, we need to call sender.wakeup(), which can only be done in the caller.
Guozhang ________________________________ From: Jay Kreps [nore...@reviews.apache.org] on behalf of Jay Kreps [boredandr...@gmail.com] Sent: Monday, July 07, 2014 1:45 PM To: Jay Kreps; kafka; Guozhang Wang Subject: Re: Review Request 23266: Fix KAFKA-1515 This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23266/ Three very minor issues: 1. It would be good to use a while loop when checking the condition in forceUpdate() (http://stackoverflow.com/questions/2536692/a-simple-scenario-using-wait-and-notify-in-java). In this case what you have actually works because the forceUpdate() method itself is called in a loop, but it would be better if forceUpdate() guaranteed its own postcondition. 2. It would be good to think through the case where a metadata request takes less than 1ms, as the avg. roundtrip I see is about .4 ms. This should not lead to waiting for maxWait if possible. One approach to this would be to use a logical version number. Another would be to have forceUpdate(long time) ensure that the metadata is at least as fresh as the given time. I think either would work. 3. Maybe awaitUpdate(int desiredVerison, long now) would be a better name? - Jay Kreps On July 7th, 2014, 5:55 p.m. UTC, Guozhang Wang wrote: Review request for kafka. By Guozhang Wang. Updated July 7, 2014, 5:55 p.m. Bugs: KAFKA-1515<https://issues.apache.org/jira/browse/KAFKA-1515> Repository: kafka Description 1. Move the waiting logic out of Metadata into KafkaProducer (KafkaConsumer would not wait on fetch metadata, so will not share this code); and wake-up sender upon waiting metadata 2. Set the refresh timestamp to now instead of 0 Diffs * clients/src/main/java/org/apache/kafka/clients/NetworkClient.java (d21f9225539b070f9b50b7a76601d80b83daf7ef) * clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java (d85ca30001dc3d6122a890c34092551654315458) * clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java (8890aa2e3ce5fffc159b3c8528138226e8c8cfd3) * clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java (37b9d1a462d42b811fffd2af3c418e3a9179f00f) * clients/src/test/java/org/apache/kafka/clients/producer/MetadataTest.java (0d7d04ca5d71d4db22da54ed2153f7c0e10cdf78) View Diff<https://reviews.apache.org/r/23266/diff/>
