MINOR: Close ZooKeeperClient if waitUntilConnected fails during construction by omkreddy · Pull Request #5411 · apache/kafka

omkreddy · 2018-07-21T12:33:11Z

This has always been an issue, but the recent upgrade to ZooKeeper
3.4.13 means that it is also an issue when an unresolvable ZK
address is used, causing some tests to leak threads.

The change in behaviour in ZK 3.4.13 is that no exception is thrown
from the ZooKeeper constructor in case of an unresolvable address.
Instead, ZooKeeper tries to re-resolve the address hoping it becomes
resolvable again. We eventually throw a
ZooKeeperClientTimeoutException, which is similar to the case
where the the address is resolvable, but ZooKeeper is not
reachable.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…rClient

omkreddy · 2018-07-21T12:35:27Z

@ijuma @rajinisivaram

Three is a zookeeper thread leak incase of unknown host address passed to zk connect string. This is due to recent ZK client lib changes. Previously zookeeper fail immediately for unknown address. In new version re-resolves hosts when connection attempts fail.

This is leading to thread leak in ServerShutdownTest.testCleanShutdownAfterFailedStartup test, ZooKeeperClientTest.testUnresolvableConnectString.

Thread leak in ServerShutdownTest.testCleanShutdownAfterFailedStartup

[2018-07-21 16:57:13,212] WARN Session 0x0 for server some.invalid.hostname.foo.bar.local:2181, unexpected error, closing socket connection and attempting reconnect (org.apache.zookeeper.ClientCnxn:1168)
java.nio.channels.UnresolvedAddressException
	at sun.nio.ch.Net.checkAddress(Net.java:101)
	at sun.nio.ch.SocketChannelImpl.connect(SocketChannelImpl.java:622)
	at org.apache.zookeeper.ClientCnxnSocketNIO.registerAndConnect(ClientCnxnSocketNIO.java:277)
	at org.apache.zookeeper.ClientCnxnSocketNIO.connect(ClientCnxnSocketNIO.java:287)
	at org.apache.zookeeper.ClientCnxn$SendThread.startConnect(ClientCnxn.java:1021)
	at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1064)

Before zookeeper-3.4.13

java.net.UnknownHostException: some.invalid.hostname.foo.bar.local: unknown error
	at java.net.Inet6AddressImpl.lookupAllHostAddr(Native Method)
	at java.net.InetAddress$2.lookupAllHostAddr(InetAddress.java:928)
	at java.net.InetAddress.getAddressesFromNameService(InetAddress.java:1323)
	at java.net.InetAddress.getAllByName0(InetAddress.java:1276)
	at java.net.InetAddress.getAllByName(InetAddress.java:1192)
	at java.net.InetAddress.getAllByName(InetAddress.java:1126)
	at org.apache.zookeeper.client.StaticHostProvider.<init>(StaticHostProvider.java:61)
	at org.apache.zookeeper.ZooKeeper.<init>(ZooKeeper.java:445)
	at org.apache.zookeeper.ZooKeeper.<init>(ZooKeeper.java:380)
	at kafka.zookeeper.ZooKeeperClient.<init>(ZooKeeperClient.scala:86)
	at kafka.zk.KafkaZkClient$.apply(KafkaZkClient.scala:1538)
	at kafka.server.KafkaServer.kafka$server$KafkaServer$$createZkClient$1(KafkaServer.scala:345)
	at kafka.server.KafkaServer.initZkClient(KafkaServer.scala:369)
	at kafka.server.KafkaServer.startup(KafkaServer.scala:202)

With zookeeper-3.4.13

kafka.zookeeper.ZooKeeperClientTimeoutException: Timed out waiting for connection while in state: CONNECTING
	at kafka.zookeeper.ZooKeeperClient$$anonfun$kafka$zookeeper$ZooKeeperClient$$waitUntilConnected$1.apply$mcV$sp(ZooKeeperClient.scala:226)
	at kafka.zookeeper.ZooKeeperClient$$anonfun$kafka$zookeeper$ZooKeeperClient$$waitUntilConnected$1.apply(ZooKeeperClient.scala:221)
	at kafka.zookeeper.ZooKeeperClient$$anonfun$kafka$zookeeper$ZooKeeperClient$$waitUntilConnected$1.apply(ZooKeeperClient.scala:221)
	at kafka.utils.CoreUtils$.inLock(CoreUtils.scala:251)
	at kafka.zookeeper.ZooKeeperClient.kafka$zookeeper$ZooKeeperClient$$waitUntilConnected(ZooKeeperClient.scala:221)
	at kafka.zookeeper.ZooKeeperClient.<init>(ZooKeeperClient.scala:95)
	at kafka.zk.KafkaZkClient$.apply(KafkaZkClient.scala:1580)
	at kafka.server.KafkaServer.kafka$server$KafkaServer$$createZkClient$1(KafkaServer.scala:348)
	at kafka.server.KafkaServer.initZkClient(KafkaServer.scala:372)
	at kafka.server.KafkaServer.startup(KafkaServer.scala:202)

ijuma

Thanks for the fix @omkreddy. Can we update the test to verify that the underlying zk is closed?

ijuma · 2018-07-21T16:35:59Z

This fix doesn't look like the right fix. We want to close zookeeper if this fails during construction only right? In other cases, the caller can decide what to do if waitUntilConnected fails.

omkreddy · 2018-07-21T17:24:12Z

Ye, we can close in constructor. Updated the PR.
Facing bit difficult to write a test. underlying ZK is not accessible incase of failure at constructor.

ijuma · 2018-07-21T18:22:13Z

+    case e: ZooKeeperClientTimeoutException =>
+      zooKeeper.close()
+      throw e
+  }


This should just be a finally instead of catch, I think.

Thinking about it some more, we actually need to shutdown the scheduler too.

Maybe we can call ZooKeeperClient.close()?

ijuma · 2018-07-21T18:36:14Z

@omkreddy I agree that writing a test is a bit challenging. The only way I can think of is to subclass ZooKeeperClient and catch the parent constructor exception in the subclass.

ijuma · 2018-07-21T21:00:34Z

+      close()
+      throw e
+  }



Thanks for updating, I still think we should use a finally block instead of catching a specific exception.

Oh wait. I was wrong, we can't use finally here. But we should probably catch a more general exception?

Yeah, we can user ZooKeeperClientException, which parent is parent class for zk excpetions

Right, as other exceptions can be thrown too. But to be honest, we might as well catch Throwable since we rethrow he exception anyway. We just want to avoid leaking the resource.

Updated the PR with Throwable . Not sure, if we can catch parent constructor exception in subclass constructor. For now, added a test based on zk thread count.

ijuma · 2018-07-22T08:10:13Z

+    case e: Throwable =>
+      close()
+      throw e
+  }


Nit: we don't need the block for try, e.g.

try waitUntilConnected(connectionTimeoutMs, TimeUnit.MILLISECONDS) catch { case e: Throwable => close() throw e }

ijuma · 2018-07-22T08:11:14Z

+    .filter(_.isAlive)
+    .map(_.getName)
+    .count(t => t.contains("SendThread()") || t.contains(hostAddress)) // Verify threadName pattern for unresolvable host zk send threads
+


Good idea for a test. Why not simply:

@Test def testUnresolvableConnectString(): Unit = { val hostAddress = "some.invalid.hostname.foo.bar.local" try { new ZooKeeperClient(hostAddress, zkSessionTimeout, connectionTimeoutMs = 10, Int.MaxValue, time, "testMetricGroup", "testMetricType") } catch { case _: ZooKeeperClientTimeoutException => assertEquals("ZooKeeper client threads still running", Set.empty, runningZkSendThreads) } } private def runningZkSendThreads: collection.Set[String] = Thread.getAllStackTraces.keySet.asScala .filter(_.isAlive) .map(_.getName) .filter(_.contains("SendThread()"))

ijuma

Thanks for the updates, LGTM.

ijuma · 2018-07-22T16:48:10Z

Merging to trunk and 2.0.

…ruction (#5411) This has always been an issue, but the recent upgrade to ZooKeeper 3.4.13 means it is also an issue when an unresolvable ZK address is used, causing some tests to leak threads. The change in behaviour in ZK 3.4.13 is that no exception is thrown from the ZooKeeper constructor in case of an unresolvable address. Instead, ZooKeeper tries to re-resolve the address hoping it becomes resolvable again. We eventually throw a `ZooKeeperClientTimeoutException`, which is similar to the case where the the address is resolvable but ZooKeeper is not reachable. Reviewers: Ismael Juma <ismael@juma.me.uk>

* apache-github/2.0: MINOR: Close ZooKeeperClient if waitUntilConnected fails during construction (apache#5411) KAFKA-6897; Prevent KafkaProducer.send from blocking when producer is closed (apache#5027)

MINOR: close Zookeeper instance incase of connect timeout in ZooKeepe…

1b5d07c

…rClient

ijuma reviewed Jul 21, 2018

View reviewed changes

Address review comment

cabce51

omkreddy force-pushed the close-zk branch from 768b467 to cabce51 Compare July 21, 2018 17:27

ijuma reviewed Jul 21, 2018

View reviewed changes

Call ZooKeeperClient close

ec05c3e

ijuma reviewed Jul 21, 2018

View reviewed changes

omkreddy added 3 commits July 22, 2018 02:37

use ZooKeeperClientException

227f889

Catch Throwable

40ddf9b

Add zookeeper SendThread count based test

1a01f88

ijuma reviewed Jul 22, 2018

View reviewed changes

omkreddy force-pushed the close-zk branch from d70a272 to 41b0be3 Compare July 22, 2018 10:43

Address review comments

a19ff80

omkreddy force-pushed the close-zk branch from 41b0be3 to a19ff80 Compare July 22, 2018 10:44

Minor tweaks

b6735f1

ijuma approved these changes Jul 22, 2018

View reviewed changes

ijuma changed the title ~~MINOR: Close Zookeeper instance incase of timeout in ZooKeeperClient initialization~~ MINOR: Release resources if exception is thrown during ZooKeeperClient construction Jul 22, 2018

ijuma changed the title ~~MINOR: Release resources if exception is thrown during ZooKeeperClient construction~~ MINOR: Close ZooKeeperClient if waitUntilConnected fails during construction Jul 22, 2018

ijuma merged commit 5db2f99 into apache:trunk Jul 22, 2018

Conversation

omkreddy commented Jul 21, 2018 • edited by ijuma Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

omkreddy commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jul 21, 2018

Uh oh!

omkreddy commented Jul 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jul 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jul 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

omkreddy commented Jul 21, 2018 •

edited by ijuma

Loading

omkreddy commented Jul 21, 2018 •

edited

Loading

ijuma Jul 21, 2018 •

edited

Loading