> On Feb. 20, 2014, 5:28 p.m., Jun Rao wrote: > > The patch doesn't apply to chunk. Could you rebase?
Wired.. I did a rebase already, would do that again. > On Feb. 20, 2014, 5:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/consumer/SimpleConsumer.scala, lines 49-51 > > <https://reviews.apache.org/r/17671/diff/2/?file=498280#file498280line49> > > > > Would it better to always call blockingChannel.disconnect() since it's > > already doing the same check there? Yes. > On Feb. 20, 2014, 5:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/producer/SyncProducer.scala, lines 129-131 > > <https://reviews.apache.org/r/17671/diff/2/?file=498282#file498282line129> > > > > Would it better to always call blockingChannel.disconnect() since it's > > already doing the same check there? Ack > On Feb. 20, 2014, 5:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaServer.scala, lines 222-223 > > <https://reviews.apache.org/r/17671/diff/2/?file=498285#file498285line222> > > > > Actual, not sure why we need this. Closing zkclient will automatically > > deregister the ephemeral node. zkClient needs to be closed as the last step of shutting down the broker, since it is needed for other modules. The deadlock is caused by the step after shutting down socket server. That is why we want to de-register the ephemeral node before shutting down the socket server. > On Feb. 20, 2014, 5:28 p.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaServer.scala, lines 136-137 > > <https://reviews.apache.org/r/17671/diff/2/?file=498285#file498285line136> > > > > Wouldn't just setting maxRetries to maxInt achieve the same thing? I am open either way, removing this logic then. - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17671/#review35013 ----------------------------------------------------------- On Feb. 19, 2014, 11:57 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17671/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2014, 11:57 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1235 > https://issues.apache.org/jira/browse/KAFKA-1235 > > > Repository: kafka > > > Description > ------- > > Also including several fixes in this patch: > > 1. Check if channel is null or not for disconnecting instead of checking the > isConnected variable, since otherwise a socket can be opened but not > connected due to an exception, and hence lead to socket leak on > channel.disconnect(). > > 2. Make sure all connect() is guarded by a disconnect in the catch clause. > This is also for socket leak prevention. > > 3. When closing the channel, check both channel and readChannel to see if > they are null separately to avoid NPE. > > 4. On shutdown, let the KafkaHealthCheck to progressively delete the > registration path in ZK to resolve the controller-shutdown-itself deadlock. > > 5. Some unit tests setup/teardown ordering. > > > Diffs > ----- > > core/src/main/scala/kafka/consumer/SimpleConsumer.scala > 6dae149adfd4bfaa0d98aa641dfc41b00fcb6162 > core/src/main/scala/kafka/network/BlockingChannel.scala > ab04b3fe0dc674455a74659ad9975458bfbbac36 > core/src/main/scala/kafka/producer/SyncProducer.scala > 041cfa59c18fa04706d28e98e58fda3937b6c992 > core/src/main/scala/kafka/server/KafkaConfig.scala > 3c3aafc2b3f06fc8f3168a8a9c1e0b08e944c1ef > core/src/main/scala/kafka/server/KafkaHealthcheck.scala > 8c69d095bfa9f79049bf9e6ec86adb7f956c2aeb > core/src/main/scala/kafka/server/KafkaServer.scala > 5e34f95e64eaf12ae7e904ffef32422a365eca86 > core/src/main/scala/kafka/utils/ZkUtils.scala > fa86bb94475dec41d5ea1a94f4eebcd5500756e6 > core/src/test/scala/unit/kafka/server/LogRecoveryTest.scala > 17a99f182f64c0608a604f0d338b1ac272399e49 > core/src/test/scala/unit/kafka/zk/ZooKeeperTestHarness.scala > 4e25b926d32e4885fa141ceaa9156c38629dd3d5 > > Diff: https://reviews.apache.org/r/17671/diff/ > > > Testing > ------- > > unit-tests > > > Thanks, > > Guozhang Wang > >
