Merge kafka-python #932

ods · 2023-10-23T11:16:47Z

Changes

Merges relevant code from kafka-python. Motivations: it's not possible to support new Python versions without changes in kafka-python. See statement from kafka-python's author.

The changes are divided on subtree split & add commits that brings original code without changes and separate commits to adopt those changes into aiokafka. This allows to see changes made into the code when reviewed commit-by-commit.

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.

git-subtree-dir: kafka git-subtree-split: c89bd5169277e2e7a6f29674d54c085a8cdbbfe3

Scenario (working branch is merge-kafka-python): git remote add kafka-python git@github.com:dpkp/kafka-python.git git fetch kafka-python git branch kafka-python-master kafka-python/master git switch kafka-python-master git subtree split --prefix=kafka -b kafka-python-kafka git switch merge-kafka-python git subtree add --squash --prefix=kafka kafka-python-kafka

git-subtree-dir: tests/kafka git-subtree-split: 1d17d3b7950b40aeeeef52281b8b7ab51622b272

Scenario (assumes kafka-python-master still exists from previous subtree add): git switch kafka-python-master git subtree split --prefix=test -b kafka-python-test git switch merge-kafka-python git subtree add --squash --prefix=tests/kafka kafka-python-test

codecov · 2023-10-23T11:24:41Z

Codecov Report

Merging #932 (66e2999) into master (00349a8) will decrease coverage by 13.35%.
The diff coverage is 69.49%.

@@             Coverage Diff             @@
##           master     #932       +/-   ##
===========================================
- Coverage   97.56%   84.21%   -13.35%     
===========================================
  Files          30       87       +57     
  Lines        5451    10085     +4634     
===========================================
+ Hits         5318     8493     +3175     
- Misses        133     1592     +1459

Flag	Coverage Δ
cext	`79.26% <69.49%> (-9.11%)`	⬇️
integration	`84.19% <69.49%> (-13.33%)`	⬇️
purepy	`83.96% <69.49%> (-13.14%)`	⬇️
unit	`51.55% <67.34%> (+13.44%)`	⬆️

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

Files	Coverage Δ
aiokafka/abc.py	`70.58% <100.00%> (-1.64%)`	⬇️
aiokafka/admin/__init__.py	`100.00% <100.00%> (ø)`
aiokafka/admin/new_partitions.py	`100.00% <100.00%> (ø)`
aiokafka/client.py	`98.16% <100.00%> (-0.01%)`	⬇️
aiokafka/consumer/consumer.py	`97.91% <100.00%> (ø)`
aiokafka/consumer/fetcher.py	`97.48% <100.00%> (ø)`
aiokafka/consumer/group_coordinator.py	`99.17% <100.00%> (ø)`
aiokafka/coordinator/protocol.py	`100.00% <100.00%> (ø)`
aiokafka/errors.py	`100.00% <100.00%> (ø)`
aiokafka/metrics/__init__.py	`100.00% <100.00%> (ø)`
... and 64 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kPsarakis · 2023-10-23T11:36:40Z

Could you also add Python 3.12 to the CI?

ods · 2023-10-23T12:27:57Z

Could you also add Python 3.12 to the CI?

I don't think it's good to mix those changes, PR is already too big

kPsarakis · 2023-10-24T11:40:27Z

I managed to run the tests with Python 3.11.5 in a conda environment. However, when I created a Python 3.12.0 environment, I got the following error with flake8 while running the tests:

make cov
black --check aiokafka/util.py aiokafka/structs.py setup.py
All done! ✨ 🍰 ✨
3 files would be left unchanged.
flake8 aiokafka tests setup.py
Traceback (most recent call last):
  File "/miniconda3/envs/aiok/bin/flake8", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/cli.py", line 22, in main
    app.run(argv)
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/application.py", line 375, in run
    self._run(argv)
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/application.py", line 363, in _run
    self.initialize(argv)
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/application.py", line 343, in initialize
    self.find_plugins(config_finder)
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/application.py", line 157, in find_plugins
    self.check_plugins = plugin_manager.Checkers(local_plugins.extension)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/plugins/manager.py", line 363, in __init__
    self.manager = PluginManager(
                   ^^^^^^^^^^^^^^
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/plugins/manager.py", line 243, in __init__
    self._load_entrypoint_plugins()
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/plugins/manager.py", line 261, in _load_entrypoint_plugins
    eps = importlib_metadata.entry_points().get(self.namespace, ())
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'EntryPoints' object has no attribute 'get'
make: *** [Makefile:22: lint] Error 1

I will investigate it a bit and get back to you.

ods · 2023-10-24T12:00:17Z

@kPsarakis Thank you for starting working on it. I suggest to concentrate on this merge using Python versions that are used in test matrix. Support for 3.12 is not just adding one version to YAML files, there is bunch of problems to solve:

aiokafka uses setup.py which is deprecated and I already face some problems, like inability to install in edit mode which is vital for local development. We have to move to pyproject.toml build (probably using hatchling) and I expect some difficulties with Cython here.
The pinned versions of libs and tools may not work. You've already stumbled upon the problem with flake, and there will be more I think.
flake8/pylint and black have different opinions on how to format code. It would be nice to replace all linters with ruff + black.
python-snappy is not updated and we may see more problems with it in 3.12. It would be good to replace it with cramjam.
I expect some deprecation warnings that we'll need to fix.

kPsarakis · 2023-10-24T14:06:53Z

aiokafka/partitioner.py

+
+
+# https://github.com/apache/kafka/blob/0.8.2/clients/src/main/java/org/apache/kafka/common/utils/Utils.java#L244
+def murmur2(data):


Is the reason for using this and not mmh3 that we do not want to depend on another external project?

Could this be a performance inhibitor? I did a small test with 1 KB data, and murmur2 is x100 slower than mmh3.hash

No reason. Would you please create a separate issue with this suggestion not to lose it? Let's keep just merge in this PR.

kPsarakis · 2023-10-24T14:37:14Z

@ods I got confused jumping to this PR from the Python 3.12 compatibility issue. That's why I focused on 3.12. I managed to run the tests with 3.12 by bumping the following requirements in the requirements-ci.txt:

flake8==6.1.0
pytest==7.4.2
pytest-cov==4.1.0
pytest-asyncio==0.21.1

And I was running them with AIOKAFKA_NO_EXTENSIONS=1 . I also had to make some changes based on some new flake8 suggestions. But I am stopping for now. After finishing this PR, I could also help with any of the points you raised.

Now, regarding this PR:

I searched for any remaining kafka-python code or anything missing, and it looks good. Also, since all tests are passing, it is relatively safe to merge, given the amount of changes in this PR.

Additionally, we use Kafka as an ingress/egress for a system we are building and aiokafka as the client library. I added git+https://github.com/ods/aiokafka@merge-kafka-python instead of aiokafka==0.8.1 in our requirements (Python 3.11), and our end-to-end tests pass without regression.

ods · 2023-10-24T15:14:18Z

Additionally, we use Kafka as an ingress/egress for a system we are building and aiokafka as the client library. I added git+https://github.com/ods/aiokafka@merge-kafka-python instead of aiokafka==0.8.1 in our requirements (Python 3.11), and our end-to-end tests pass without regression.

Thanks, testing with real-life scenarios are really valuable for this change!

aio-libs/aiokafka#932

ods added 21 commits October 21, 2023 20:54

Speed up local testing: don't regenerate certs

ee20b37

Squashed 'kafka/' content from commit c89bd516

86562e7

git-subtree-dir: kafka git-subtree-split: c89bd5169277e2e7a6f29674d54c085a8cdbbfe3

Remove kafka-python from dependencies

3f3c169

Squashed 'tests/kafka/' content from commit 1d17d3b7

90a8851

git-subtree-dir: tests/kafka git-subtree-split: 1d17d3b7950b40aeeeef52281b8b7ab51622b272

Adjust imports in kafka-python tests

c220efc

De-dup: record packages

e7c11e6

Untie ConsumerRebalanceListener

29526ce

Merge admin

97386fc

Merge errors

adde152

Move coordinator

04b7bff

Move partitioner

f311fb0

Merge cluster

81e5bf2

Merge structs

59fbd73

Move oauth

acff7a5

Move metrics

b994339

Merge protocol

91759b8

Move codec

8c4cd40

Merge util

6d70164

Replace future and final clean-up

a2ec341

ods requested a review from tvoinarovskyi October 23, 2023 11:18

ods mentioned this pull request Oct 23, 2023

Python 3.12.0 compatibility issue #928

Closed

Remove dead code

66e2999

kPsarakis reviewed Oct 24, 2023

View reviewed changes

ods merged commit 88c6945 into aio-libs:master Nov 8, 2023
23 of 25 checks passed

timkpaine added a commit to regro-cf-autotick-bot/aiokafka-feedstock that referenced this pull request Dec 5, 2023

remove kafka-python dep

f6b6f1b

aio-libs/aiokafka#932

ods mentioned this pull request Mar 16, 2024

More types #881

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge kafka-python #932

Merge kafka-python #932

ods commented Oct 23, 2023

codecov bot commented Oct 23, 2023 •

edited

Loading

kPsarakis commented Oct 23, 2023

ods commented Oct 23, 2023

kPsarakis commented Oct 24, 2023

ods commented Oct 24, 2023

kPsarakis Oct 24, 2023

ods Oct 24, 2023

kPsarakis commented Oct 24, 2023

ods commented Oct 24, 2023



		# https://github.com/apache/kafka/blob/0.8.2/clients/src/main/java/org/apache/kafka/common/utils/Utils.java#L244
		def murmur2(data):

Merge kafka-python #932

Merge kafka-python #932

Conversation

ods commented Oct 23, 2023

Changes

Checklist

codecov bot commented Oct 23, 2023 • edited Loading

Codecov Report

kPsarakis commented Oct 23, 2023

ods commented Oct 23, 2023

kPsarakis commented Oct 24, 2023

ods commented Oct 24, 2023

kPsarakis Oct 24, 2023

Choose a reason for hiding this comment

ods Oct 24, 2023

Choose a reason for hiding this comment

kPsarakis commented Oct 24, 2023

ods commented Oct 24, 2023

codecov bot commented Oct 23, 2023 •

edited

Loading