Skip to content

Navigation Menu

Sign in
Appearance settings
Sign up
Appearance settings
This repository was archived by the owner on Mar 24, 2021. It is now read-only.

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

#939
Open
saich wants to merge 3 commits into
Parsely:masterParsely/pykafka:masterfrom
Instamojo:bugfix/reconnect-socketsInstamojo/pykafka:bugfix/reconnect-socketsCopy head branch name to clipboard
Open

Socket reconnection on retries of JoinGroup, SyncGroup, LeaveGroup#939
saich wants to merge 3 commits into
Parsely:masterParsely/pykafka:masterfrom
Instamojo:bugfix/reconnect-socketsInstamojo/pykafka:bugfix/reconnect-socketsCopy head branch name to clipboard

Conversation

@saich

@saich saich commented Apr 26, 2019

Copy link
Copy Markdown
Contributor

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

saich added 3 commits April 18, 2019 08:27
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

codecov-io commented Apr 26, 2019

Copy link
Copy Markdown

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
Copy Markdown
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.