Implement KIP-345 in aiokafka

Open patkivikram opened this issue 3 years ago • 29 comments

Changes

Fixes #https://github.com/aio-libs/aiokafka/issues/680

Implements KIP-345 in aiokafka

Checklist

[x] I think the code is well written
[ ] Unit tests for the changes exist
[x] Documentation reflects the changes
[ ] Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> (e.g. 588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the PR
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

Nov 03 '20 22:11 patkivikram

This pull request introduces 3 alerts when merging 48ab7a390d4bb9c317d3a817dcd367c1225e8ffd into f7f55b19b43b084edc844c3531a570d233d37912 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Unused import
1 for 'import *' may pollute namespace

Nov 03 '20 22:11 lgtm-com[bot]

Codecov Report

Merging #682 (306ae6e) into master (f4ac354) will increase coverage by 0.00%. The diff coverage is 97.91%.

@@           Coverage Diff           @@
##           master     #682   +/-   ##
=======================================
  Coverage   97.99%   98.00%           
=======================================
  Files          29       29           
  Lines        5193     5207   +14     
=======================================
+ Hits         5089     5103   +14     
  Misses        104      104

Flag	Coverage Δ
cext	`87.70% <97.91%> (-0.01%)`	:arrow_down:
integration	`97.96% <97.91%> (+<0.01%)`	:arrow_up:
purepy	`97.52% <97.91%> (+0.08%)`	:arrow_up:
unit	`51.52% <70.83%> (+0.07%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiokafka/producer/producer.py	`98.50% <75.00%> (ø)`
aiokafka/client.py	`98.15% <100.00%> (+0.02%)`	:arrow_up:
aiokafka/conn.py	`94.10% <100.00%> (ø)`
aiokafka/consumer/consumer.py	`97.90% <100.00%> (ø)`
aiokafka/consumer/fetcher.py	`97.49% <100.00%> (+<0.01%)`	:arrow_up:
aiokafka/consumer/subscription_state.py	`100.00% <100.00%> (ø)`
aiokafka/producer/message_accumulator.py	`98.90% <100.00%> (+0.01%)`	:arrow_up:

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 f4ac354...bbbfc4b. Read the comment docs.

Nov 09 '20 15:11 codecov[bot]

@tvoinarovskyi does this look okay?

Nov 12 '20 00:11 patkivikram

All committers have signed the CLA.

Nov 16 '20 10:11 CLAassistant

This pull request introduces 3 alerts when merging 8a447b318a050457bf76c5eb0f68876aed78912c into f4ac3544c547115ea664fb5ea19fc69766cf75b8 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Unused import
1 for 'import *' may pollute namespace

Nov 25 '20 16:11 lgtm-com[bot]

This pull request introduces 3 alerts when merging 2801e2493f3156b28d05f40a74efb3f4b74a958e into f4ac3544c547115ea664fb5ea19fc69766cf75b8 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Unused import
1 for 'import *' may pollute namespace

Nov 25 '20 16:11 lgtm-com[bot]

This pull request introduces 3 alerts when merging 4d908f4408efc680495ed58697acb5cec3087f5e into f4ac3544c547115ea664fb5ea19fc69766cf75b8 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Unused import
1 for 'import *' may pollute namespace

Nov 25 '20 17:11 lgtm-com[bot]

This pull request introduces 3 alerts when merging aac8e279b4e54a5e11ced06ed8629ce41f36bb5b into f4ac3544c547115ea664fb5ea19fc69766cf75b8 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Unused import
1 for 'import *' may pollute namespace

Nov 25 '20 19:11 lgtm-com[bot]

This pull request introduces 3 alerts when merging 63c5f0255f0d36631db480cf56742e75cdff5fd5 into f4ac3544c547115ea664fb5ea19fc69766cf75b8 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Unused import
1 for 'import *' may pollute namespace

Nov 30 '20 17:11 lgtm-com[bot]

This pull request introduces 3 alerts when merging 757a79378c61a9c0cc618f4f4af882323dd79fb8 into 07e9bd3a3511cdb55fc2dafcda1dc9eea5bca88c - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method
1 for Unused import
1 for 'import *' may pollute namespace

Dec 04 '20 22:12 lgtm-com[bot]

unsubscribe On Wednesday, November 4, 2020, 04:18:33 AM GMT+5:30, Vikram Patki [email protected] wrote:

Changes

Fixes ##680

Implements KIP-345 in aiokafka

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
Add a new news fragment into the CHANGES folder
- name it <issue_id>. (e.g. 588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the PR
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

You can view, comment on, or merge this pull request online at:

https://github.com/aio-libs/aiokafka/pull/682

Commit Summary

Implement KIP-345 in aiokafka

File Changes

M aiokafka/client.py (2)
A aiokafka/consumer/assignors.py (23)
M aiokafka/consumer/consumer.py (5)
M aiokafka/consumer/group_coordinator.py (158)
M aiokafka/errors.py (8)
A aiokafka/protocol/group.py (75)

Patch Links:

https://github.com/aio-libs/aiokafka/pull/682.patch
https://github.com/aio-libs/aiokafka/pull/682.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

Dec 05 '20 03:12 reddyanilkumar

This pull request introduces 1 alert when merging 6d26defed8cd237720e7b184bc2cc3a24c960936 into 07e9bd3a3511cdb55fc2dafcda1dc9eea5bca88c - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method

Dec 05 '20 22:12 lgtm-com[bot]

Hi @patkivikram, looks great, did you use some linter for the files here? It's really hard to review because of the big amount of changes unrelated to logic. Would it be hard to either do a separate PR if the linter used is "black" or revert the lint changes? Sorry for the trouble.

Dec 06 '20 22:12 tvoinarovskyi

This pull request introduces 1 alert when merging 6790467eed73d1cd6320ecd406d87a9e788f3086 into 4fbea5c786443a219dfecbf73241884593297882 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method

Dec 07 '20 22:12 lgtm-com[bot]

@tvoinarovskyi let me know if this looks good

Dec 08 '20 02:12 patkivikram

This pull request introduces 1 alert when merging 592859ff6db2ec04a4a56039658399af9b2e6b11 into 4fbea5c786443a219dfecbf73241884593297882 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method

Dec 08 '20 14:12 lgtm-com[bot]

This pull request introduces 1 alert when merging 71acb943bb50a1d528f799dd0db1204a86603d2e into 4fbea5c786443a219dfecbf73241884593297882 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method

Dec 08 '20 15:12 lgtm-com[bot]

This pull request introduces 1 alert when merging 74dd9bee367fdb09c9b7ee5b56e72da392a517ec into 4fbea5c786443a219dfecbf73241884593297882 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method

Dec 09 '20 19:12 lgtm-com[bot]

This pull request introduces 1 alert when merging bbbfc4bf4ed02c0a25b4a86c8ec7afa5b5867975 into 306ae6e6d78d10efb64d36f57c6ef1af3d5fb682 - view on LGTM.com

new alerts:

1 for Signature mismatch in overriding method

Dec 11 '20 21:12 lgtm-com[bot]

Hi all, just wondering if this PR has an ETA?

Jan 27 '21 09:01 deed02392

What's left to do to get this merged? Can I help? @tvoinarovskyi - any insights?

Feb 24 '21 13:02 forsberg

Hi, thanks for the work on this, now it's rally clear and I would love to have this merged. The only issue I have is the abscense of tests and I am not familiar with the Static group management enough. Could you share how did you test the change, can you try preparing a test to reproduce the membership change? Or I could try to do it if I know how to reproduce it manually.

Feb 27 '21 17:02 tvoinarovskyi

Here's how I reproduce it manually (which I'm not sure if you can automate...it's a little messy):

Assume a topic with multiple partitions and multiple consumers.
In non-KIP-345-mode
- When one consumer leaves, the other consumers immediately rebalance.
- When the original consumer returns, another rebalance occurs and the original consumer gets partitions that may or may not include its original assignments, depending on the negotiation in the rebalance.
In KIP-345-mode
- When one consumer leaves, no rebalance occurs until after that consumer's session timeout expires.
- If the original consumer rejoins the group before its session timeout expiration, the consumer is assigned the same partitions it had before leaving.

I've tested this manually by running consumers locally and watching what happens when I kill or stop one of them, which I acknowledge is not the greatest way to test something.