Skip to content

Conversation

@JoshRosen
Copy link
Contributor

In PersistenceEngineSuite, we do not call close() on the PersistenceEngine at the end of the test. For the ZooKeeperPersistenceEngine, this causes us to leak a ZooKeeper client, causing the logs of unrelated tests to be periodically spammed with connection error messages from that client:

15/11/20 05:13:35.789 pool-1-thread-1-ScalaTest-running-PersistenceEngineSuite-SendThread(localhost:15741) INFO ClientCnxn: Opening socket connection to server localhost/127.0.0.1:15741. Will not attempt to authenticate using SASL (unknown error)
15/11/20 05:13:35.790 pool-1-thread-1-ScalaTest-running-PersistenceEngineSuite-SendThread(localhost:15741) WARN ClientCnxn: Session 0x15124ff48dd0000 for server null, unexpected error, closing socket connection and attempting reconnect
java.net.ConnectException: Connection refused
    at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
    at sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:739)
    at org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:350)
    at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1068)

This patch fixes this by using a finally block.

@JoshRosen
Copy link
Contributor Author

Reviewable's diff makes it clear that this patch is 99% whitespace changes: https://reviewable.io/reviews/apache/spark/9864

@SparkQA
Copy link

SparkQA commented Nov 20, 2015

Test build #46428 has finished for PR 9864 at commit ea9d063.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

/cc @zsxwing for review.

@JoshRosen
Copy link
Contributor Author

You can look at https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/46428/artifact/core/target/ to see that this ended up fixing those spurious log messages.

@zsxwing
Copy link
Member

zsxwing commented Nov 20, 2015

Reviewable's diff makes it clear that this patch is 99% whitespace changes: https://reviewable.io/reviews/apache/spark/9864

You can also use https://github.com/apache/spark/pull/9864/files?w=1 to ignore space changes :)

@zsxwing
Copy link
Member

zsxwing commented Nov 20, 2015

LGTM

@JoshRosen
Copy link
Contributor Author

Woah, cool! I didn't know about that GitHub feature. I'm going to merge this into master and branch-1.6.

asfgit pushed a commit that referenced this pull request Nov 20, 2015
…Suite tests

In PersistenceEngineSuite, we do not call `close()` on the PersistenceEngine at the end of the test. For the ZooKeeperPersistenceEngine, this causes us to leak a ZooKeeper client, causing the logs of unrelated tests to be periodically spammed with connection error messages from that client:

```
15/11/20 05:13:35.789 pool-1-thread-1-ScalaTest-running-PersistenceEngineSuite-SendThread(localhost:15741) INFO ClientCnxn: Opening socket connection to server localhost/127.0.0.1:15741. Will not attempt to authenticate using SASL (unknown error)
15/11/20 05:13:35.790 pool-1-thread-1-ScalaTest-running-PersistenceEngineSuite-SendThread(localhost:15741) WARN ClientCnxn: Session 0x15124ff48dd0000 for server null, unexpected error, closing socket connection and attempting reconnect
java.net.ConnectException: Connection refused
	at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
	at sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:739)
	at org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:350)
	at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1068)
```

This patch fixes this by using a `finally` block.

Author: Josh Rosen <joshrosen@databricks.com>

Closes #9864 from JoshRosen/close-zookeeper-client-in-tests.

(cherry picked from commit 89fd9bd)
Signed-off-by: Josh Rosen <joshrosen@databricks.com>
@asfgit asfgit closed this in 89fd9bd Nov 20, 2015
@JoshRosen JoshRosen deleted the close-zookeeper-client-in-tests branch November 20, 2015 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants