Timeout idle connections via connections_max_idle_ms #1068
Conversation
dpkp
force-pushed
the
timeout_idle_connections
branch
3 times, most recently
from
fd5a9f4 to
b1b8eb3
Compare
|
You have a bunch of Also, I was a little lost on the implementation and the interplay with |
|
I chose Re consumer and producer, connections are created as needed in normal course. Disconnecting idle connections should not have any impact. |
dpkp
force-pushed
the
timeout_idle_connections
branch
from
b1b8eb3 to
e5117a2
Compare
|
|
||
| def is_expired(self, conn_id): | ||
| if conn_id not in self.lru_connections: | ||
| return None |
There was a problem hiding this comment.
Should this be False instead of None to be consistent for the return type bool?
There was a problem hiding this comment.
I chose None here to be "falsey" but distinguishable from False since the connection in this case is not "unexpired" but rather "unknown".
There was a problem hiding this comment.
That can be confusing as the next return is bool type.
In which case that one connection would not be managed by this and None is returned?
| if time.time() < self.next_idle_close_check_time: | ||
| return None | ||
|
|
||
| if not len(self.lru_connections): |
There was a problem hiding this comment.
redundant len() check: if not self.lru_connections:
| def update_next_idle_close_check_time(self, ts): | ||
| self.next_idle_close_check_time = ts + self.connections_max_idle | ||
|
|
||
| def poll_expired_connection(self): |
There was a problem hiding this comment.
Personally, I would return a generator iterator using yield for all expired connections instead of None. In the call to this function, it would iterate through and close them.
There was a problem hiding this comment.
we could do that, but I chose to stay fairly close to the java client logic just to be safe. I believe this implementation works, and I'm also skeptical that there's much performance optimization to be gained given the relative infrequency with which connection timeouts are likely to occur.
There was a problem hiding this comment.
I didn't check Java client implementation. If that is the case, I would also try to be close to Java' client behavior. It is not to gain performance but convenient.
| @@ -266,7 +266,7 @@ class KafkaProducer(object): | |||
| 'linger_ms': 0, | |||
| 'partitioner': DefaultPartitioner(), | |||
| 'buffer_memory': 33554432, | |||
| 'connections_max_idle_ms': 600000, # not implemented yet | |||
| 'connections_max_idle_ms': 9 * 60 * 1000, # not implemented yet | |||
There was a problem hiding this comment.
Should remove the "not implemented yet" comment? 😉
There was a problem hiding this comment.
I think there's a similar comment in the consumer that also needs updating
There was a problem hiding this comment.
Yes -- this is fixed up in master
Fixes #548 #1049 #1050