----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21937/#review44850 -----------------------------------------------------------
Good to see this patch. Looks great, I have few minor review comments. Also, at least one test seems to hang (kafka.api.ProducerFailureHandlingTest) clients/src/main/java/org/apache/kafka/clients/ClientRequest.java <https://reviews.apache.org/r/21937/#comment79474> LICENSE header is missing here and in all the new files clients/src/main/java/org/apache/kafka/clients/ClientRequest.java <https://reviews.apache.org/r/21937/#comment79472> typo: milliseonds clients/src/main/java/org/apache/kafka/clients/ClientResponse.java <https://reviews.apache.org/r/21937/#comment79473> Could you add javadoc for the constructor and also for the class? clients/src/main/java/org/apache/kafka/clients/KafkaClient.java <https://reviews.apache.org/r/21937/#comment79439> Will be good to specify what happens with requests that are sent on non ready connections clients/src/main/java/org/apache/kafka/clients/NetworkClient.java <https://reviews.apache.org/r/21937/#comment79440> nodes->node's clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/21937/#comment79478> Now we will end up with potentially two Senders - one for the producer's state machine and another for the consumer's state machine. Can we rename this one to sth like ProduerSender? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/21937/#comment79436> Should we call client.close() here to close the selector? clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/21937/#comment79435> nowMs->now clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java <https://reviews.apache.org/r/21937/#comment79479> nowMs->now clients/src/main/java/org/apache/kafka/common/network/Selector.java <https://reviews.apache.org/r/21937/#comment79433> This import is unused. Probably we can just revert the changes to this file clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java <https://reviews.apache.org/r/21937/#comment79430> This API is unused. Can we remove it? clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java <https://reviews.apache.org/r/21937/#comment79431> This API seems unused as well - Neha Narkhede On June 3, 2014, 9:33 p.m., Jay Kreps wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21937/ > ----------------------------------------------------------- > > (Updated June 3, 2014, 9:33 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1316 > https://issues.apache.org/jira/browse/KAFKA-1316 > > > Repository: kafka > > > Description > ------- > > KAFKA-1316 Refactor a reusable NetworkClient interface out of Sender. > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/ClientRequest.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/ClientResponse.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/ClusterConnectionStates.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/ConnectionState.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/KafkaClient.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/NodeConnectionState.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > d15562a968d9e4b08f26b8d30986881adfe29e31 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > 9b1f5653548ba90defdae43940a5554066770b0a > clients/src/main/java/org/apache/kafka/common/network/Selector.java > 3e358985ed72a894a71d683acc7460695d6f2056 > clients/src/main/java/org/apache/kafka/common/protocol/types/Schema.java > 68b8827f3bdd64580e1b443fce5b8c63152dd94a > clients/src/main/java/org/apache/kafka/common/record/MemoryRecords.java > 428968cd38a7b12991f87868bf759926ff7e594e > clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java > 6fa4a58f5f9792776a647e8f682d7faadc0d1556 > clients/src/test/java/org/apache/kafka/clients/MockClient.java PRE-CREATION > clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java > PRE-CREATION > > clients/src/test/java/org/apache/kafka/clients/producer/RecordAccumulatorTest.java > c4072ae90fb58101a67f83054fbe0b8349e71c2e > clients/src/test/java/org/apache/kafka/clients/producer/SenderTest.java > 3ef692ca3e9fb83868a9da6f30c0705bb3d0aed2 > clients/src/test/java/org/apache/kafka/common/utils/MockTime.java > cda8e644587aad9a8c9c96c222edc1ba27de1fb0 > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > cd4ca2fa77763b090c6ad4ba4a5d46a6a8b76698 > > Diff: https://reviews.apache.org/r/21937/diff/ > > > Testing > ------- > > > Thanks, > > Jay Kreps > >
