Skip to content
This repository has been archived by the owner on Mar 24, 2021. It is now read-only.

Socket reconnection on retries of JoinGroup, SyncGroup, LeaveGroup #939

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

saich
Copy link
Contributor

@saich saich commented Apr 26, 2019

Today, if JoinGroup sees a socket disconnection, it will retry again without reconnecting the connection which results in subsequent retries all to fail.

This change will ensure that we attempt to reconnect the connection if see a disconnected connection while selecting a unique RequestHandler to use for this request.

Alternative approaches that can be considered:

  • Reconnect automatically in RequestHandler.request before calling self.shared.requests.put
  • Reconnect automatically in BrokerConnection.request if required

Sync with upstream master
`Broker._get_unique_req_handler` is responsible to identify which
`BrokerConnection` & which `RequestHandler` objects will be used for
`JoinGroup`, `SyncGroup`, `LeaveGroup` etc.

This change will ensure that this method doesn't reuse a disconnected
connection without attempting to reconnect.
@codecov-io
Copy link

codecov-io commented Apr 26, 2019

Codecov Report

Merging #939 into master will increase coverage by 0.09%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
+ Coverage   83.32%   83.41%   +0.09%     
==========================================
  Files          36       36              
  Lines        3807     3811       +4     
  Branches      562      563       +1     
==========================================
+ Hits         3172     3179       +7     
+ Misses        489      486       -3     
  Partials      146      146
Impacted Files Coverage Δ
pykafka/managedbalancedconsumer.py 84.1% <100%> (ø) ⬆️
pykafka/broker.py 88.13% <60%> (-5.51%) ⬇️
pykafka/connection.py 83.16% <0%> (-1.99%) ⬇️
pykafka/topic.py 80.41% <0%> (-1.04%) ⬇️
pykafka/balancedconsumer.py 91.22% <0%> (+1.16%) ⬆️
pykafka/simpleconsumer.py 86.6% <0%> (+1.56%) ⬆️
pykafka/cluster.py 71.89% <0%> (+1.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6599354...238bcc8. Read the comment docs.

@emmettbutler
Copy link
Contributor

Thanks for the contribution. I think your first alternate suggestion of putting this functionality in RequestHandler might be a better way to maintain encapsulation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants