Skip to content

Do not purge inUse connections #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 29, 2017
Merged

Conversation

zhenlineo
Copy link
Contributor

Do not purge inUse connections when connection error happens or new routing table is available

Instead we deactive the server (a.k.a. closing all idle connections) in connection pool.

When an connection failure happens, the connection server will be removed from the routing table and all idle connections with this server will be closed in the connection pool.

When a new routing table is available, all idle connections towards the servers that are not in the new routing table will be removed. If the server has no inUse connections, then this server will also be totally removed from the connection pool.

Note:

  • There is no extra protection to against acquire to create a new connection with an inactive server except that the server should not be in the routing table and therefore should not be used to acquire new connections.
  • When error happens on a connection, we will deactivate the server but we will probably not remove all server's connections from connection pool as the failed connection is highly still inUse by the broken session.

@zhenlineo zhenlineo requested a review from technige November 23, 2017 16:24
@zhenlineo zhenlineo force-pushed the 1.5-purge-idle branch 4 times, most recently from b1cd45c to 0abb9f8 Compare November 24, 2017 12:52
@zhenlineo zhenlineo changed the base branch from 1.6 to 1.5 November 27, 2017 11:17
@@ -140,6 +143,9 @@ def update(self, new_routing_table):
self.last_updated_time = self.timer()
self.ttl = new_routing_table.ttl

def servers(self):
return set(self.routers.elements() + self.writers.elements() + self.readers.elements())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be return set(self.routers) | set(self.writers) | set(self.readers) and there is no need to add an elements accessor on OrderedSet.

@@ -81,6 +81,9 @@ def replace(self, elements=()):
e.clear()
e.update(OrderedDict.fromkeys(elements))

def elements(self):
return list(self._elements)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed - see below.

conn.close()
except IOError:
pass
if len(connections) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not connections

Zhen added 2 commits November 29, 2017 13:44
…outing table is available

Instead we deactive the server (a.k.a. closing all idle connections) in connection pool.

When an connection failure happens, the connection server will be removed from the routing table and all idle connections with this server will be closed in the connection pool.
When a new routing table is available, all idle connections towards the servers that are not in the new routing table will be removed. If the server has no inUse connections, then this server will also be totally removed from the connection pool.

Note: there is no extra protection to against `acquire` to create a new connection with an inactive server except that the server should not be in the routing table and therefore should not be used to `acquire` new connections.
When error happens on a connection, we will deactivate the server but we will probably not remove all server's connections from connection pool as the failed connection is highly still inUse by the broken session.
@zhenlineo zhenlineo merged commit ad928c5 into neo4j:1.5 Nov 29, 2017
@zhenlineo zhenlineo deleted the 1.5-purge-idle branch November 29, 2017 13:14
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.

2 participants