Implement KIP-345 in aiokafka (rebase of #827) #941

joshuaherrera · 2023-11-15T17:31:31Z

Changes

Fixes #680

This is a rebase of #827 that adds test for KIP-345 functionality - See #827 (comment)

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>.<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.

suppressing lgtm warning

joshuaherrera · 2023-11-15T17:33:43Z

@ods Here's the PR for the tests, I'll work on getting merge conflicts fixed on my end since it's only two files, hopefully by EOD today.

Edit: Merge conflicts resolved.

aiokafka/consumer/group_coordinator.py

aiokafka/consumer/assignors.py

aiokafka/consumer/group_coordinator.py

Co-authored-by: Denis Otkidach <denis.otkidach@gmail.com>

… argument

codecov · 2023-11-29T17:12:27Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d7201c1) 94.89% compared to head (5c60f63) 94.91%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
tests/test_consumer.py	97.58%	2 Missing and 1 partial ⚠️
aiokafka/consumer/group_coordinator.py	95.23%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #941      +/-   ##
==========================================
+ Coverage   94.89%   94.91%   +0.01%     
==========================================
  Files         106      106              
  Lines       16136    16318     +182     
  Branches     2592     2615      +23     
==========================================
+ Hits        15313    15488     +175     
- Misses        549      554       +5     
- Partials      274      276       +2

Flag	Coverage Δ
cext	`91.59% <95.67%> (+0.01%)`	⬆️
integration	`94.56% <95.67%> (-0.01%)`	⬇️
purepy	`94.42% <95.67%> (+<0.01%)`	⬆️
unit	`44.90% <20.19%> (-0.30%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tvoinarovskyi · 2023-11-29T17:20:28Z

Hi joshuaherrera,

Thanks for the PR, will be joining the review. Really nice, clean PR, thanks for the contribution. Added a few small points to consider.
Could you do a rebase to master, seems like we have a few broken CI runs due to upstream changes?

tvoinarovskyi · 2023-11-29T17:08:31Z

aiokafka/consumer/group_coordinator.py

+            while try_join:
+                try_join = False
+
+                if self._api_version < (0, 10, 1):


nit: Could we abstract out this into a function?

This depends on the KIP-394 decision, since this is tied to that. If we move that to another PR than this would revert to how we currently handle this in master.

tvoinarovskyi · 2023-11-29T17:10:13Z

aiokafka/consumer/group_coordinator.py

@@ -1202,46 +1217,67 @@ async def perform_group_join(self):
                metadata = metadata.encode()
            group_protocol = (assignor.name, metadata)
            metadata_list.append(group_protocol)
+            # for KIP-394 we may have to send a second join request


This sounds like a completely different effort, maybe worth separating out to another PR?

The bulk of this addition is captured in the new while loop wrapping lines 1222-1279, and the newly added MemberIdRequired error. KIP-394 introduces the MemberIdRequired error, and according to the KIP, if the client gets this error they should retry the join with the previous member id.

I'd prefer to leave it in since the original author thought it necessary and the KIP itself calls out that this is "an important complement to KIP-345" but if you think this should be moved to it's own PR (and would delay this PR) I can remove the code related to KIP-394 from this PR.

@tvoinarovskyi Any updates on next steps? If you or @ods think it's best to remove this and leave it for another PR, I'm happy to do so, since it seems like this is the last unresolved thread for this PR. I don't want to hold this up unnecessarily.

tvoinarovskyi · 2023-11-29T17:13:48Z

tests/test_consumer.py

+        # partitions after rebalance
+        all_partitions = frozenset(list(c1_partitions) + list(c2_partitions))
+        await consumer2.stop()
+        await asyncio.sleep(25)


Could we document here exactly the reason this is 25s? As I understand it is derived from default timeouts, maybe we can make it as low as 10s?
The reason I am asking is that CI has limited time to execute and stops like those increase runtime constantly. A better approach would be to poll for results, for example, wait for every second till listener1.revoke_mock.call_count > 2.

I'll make the polling change as recommended.

tvoinarovskyi · 2023-11-29T17:21:30Z

tests/test_consumer.py

+
+        # stop consumer2, which will trigger yet another rebalance
+        await consumer2.stop()
+        await asyncio.sleep(15)


Same as above

tests/test_consumer.py

aiokafka/consumer/group_coordinator.py

tartieret · 2023-12-12T17:02:01Z

This is very exciting ! Thanks a lot for taking care of this. Do you have any idea when this could be released?

ods · 2023-12-13T09:03:21Z

@tartieret Please try it out and provide feedback whether it works as expected.

tartieret · 2023-12-13T19:51:46Z

@tartieret Please try it out and provide feedback whether it works as expected.

I'll have someone in my team look at this over the next few days

igulati · 2023-12-28T21:52:26Z

@tartieret Please try it out and provide feedback whether it works as expected.

hey @ods , replying on behalf of @tartieret, I just tested these changes and they seems to be working for me.

patkivikram and others added 15 commits October 30, 2023 11:51

Implement KIP-345 in aiokafka

2dcf7b7

fixing linting errors

d02ddd9

fixing linting errors

fc83b3b

fixing linting errors

052b66f

Update tests.yml

4aebeb1

Fix linting errors

5c55d46

Linting fixed, tests still failing

39f8fa3

fixed tests.

0c4cf27

Undoing a lot of linting

f1ab871

last few lints

99cc6f0

Update assignors.py

5a90fc7

suppressing lgtm warning

fix linting

1695902

fix lgtm exception

8017671

fix trailing space

77747f2

add KIP-345 tests, remove broker version check

5d807f2

joshuaherrera added 3 commits November 15, 2023 10:46

Merge 'upstream/master' into kip-345, resolve conflicts

54d6870

use aiokafka AbstractPartitionAssignor

a570839

only test KIP-345 mode with valid Kafka versions

46f0b40

ods reviewed Nov 19, 2023

View reviewed changes

aiokafka/consumer/group_coordinator.py Outdated Show resolved Hide resolved

aiokafka/consumer/assignors.py Outdated Show resolved Hide resolved

aiokafka/consumer/group_coordinator.py Outdated Show resolved Hide resolved

aiokafka/consumer/group_coordinator.py Outdated Show resolved Hide resolved

joshuaherrera and others added 3 commits November 20, 2023 10:15

Update aiokafka/consumer/group_coordinator.py

c0b934f

Co-authored-by: Denis Otkidach <denis.otkidach@gmail.com>

remove AbstractStaticPartitionAssignor

7ffca59

refactor _perform_assignment to use a JoinGroupResponse class as it's…

b8b7ffd

… argument

joshuaherrera requested a review from ods November 28, 2023 23:41

tvoinarovskyi self-requested a review November 29, 2023 17:04

tvoinarovskyi reviewed Nov 29, 2023

View reviewed changes

joshuaherrera added 2 commits November 29, 2023 14:10

Merge remote-tracking branch 'upstream/master' into kip-345

140d115

poll periodically in kip-345 tests

d9bc186

ods reviewed Nov 30, 2023

View reviewed changes

tests/test_consumer.py Outdated Show resolved Hide resolved

update tests to use async_timeout

d430920

ods reviewed Dec 2, 2023

View reviewed changes

aiokafka/consumer/group_coordinator.py Outdated Show resolved Hide resolved

ods reviewed Dec 2, 2023

View reviewed changes

aiokafka/consumer/group_coordinator.py Outdated Show resolved Hide resolved

update isinstance function, use JoinGroupResponse_v5 for check

5c60f63

joshuaherrera requested review from tvoinarovskyi and ods December 4, 2023 17:24

ods approved these changes Dec 4, 2023

View reviewed changes

ods merged commit bc14e6d into aio-libs:master Dec 7, 2023
29 of 31 checks passed

ods mentioned this pull request Dec 7, 2023

Implement KIP-345 in aiokafka (rebase of #682) #827

Closed

4 tasks

This was referenced Jan 9, 2024

Draft: Aiokafka kip 345 smaxtec/faust#13

Open

enable kafka static-partition-assignment (KIP-345) faust-streaming/faust#600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement KIP-345 in aiokafka (rebase of #827) #941

Implement KIP-345 in aiokafka (rebase of #827) #941

joshuaherrera commented Nov 15, 2023

joshuaherrera commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

tvoinarovskyi commented Nov 29, 2023

tvoinarovskyi Nov 29, 2023

joshuaherrera Nov 29, 2023

tvoinarovskyi Nov 29, 2023

joshuaherrera Nov 29, 2023

joshuaherrera Dec 5, 2023

tvoinarovskyi Nov 29, 2023

joshuaherrera Nov 29, 2023

tvoinarovskyi Nov 29, 2023

tartieret commented Dec 12, 2023

ods commented Dec 13, 2023

tartieret commented Dec 13, 2023

igulati commented Dec 28, 2023

Implement KIP-345 in aiokafka (rebase of #827) #941

Implement KIP-345 in aiokafka (rebase of #827) #941

Conversation

joshuaherrera commented Nov 15, 2023

Changes

Checklist

joshuaherrera commented Nov 15, 2023 • edited Loading

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

tvoinarovskyi commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tartieret commented Dec 12, 2023

ods commented Dec 13, 2023

tartieret commented Dec 13, 2023

igulati commented Dec 28, 2023

joshuaherrera commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading