Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify cluster metadata #942

Closed
wants to merge 3 commits into from

Conversation

uchiiii
Copy link

@uchiiii uchiiii commented Nov 16, 2023

Changes

Fixes #943

Peformance

I reported #943 and re-measured the performance in the same environment after this improvement. Here is the summary of both before and after.

  • before
$ ./run-benchmarks.sh
Running Tests for Kafka-Python
Starting benchmark for Kafka-Python producer.
Kafka-Python producer Results:
Number of Runs: 5, Number of messages: 100000, Message Size: 100 bytes.
Average Time for 100000 messages: 7.028003358840943 seconds.
Messages / sec: 14228.792289093612
MB / sec : 1.3569633759587871
===================
Running Tests for AIOKafka
/root/workspace/news/kafka-client-benchmarks/aiokafka-benchmark.py:40: DeprecationWarning: The loop argument is deprecated since 0.7.1, and scheduled for removal in 0.9.0
  producer = AIOKafkaProducer(loop=loop, **producer_config)
Starting benchmark for AIOKafka Producer.
AIOKafka Producer Results:                   
Number of Runs: 5, Number of messages: 100000, Message Size: 100 bytes.
Average Time for 100000 messages: 54.26867785453796 seconds.
Messages / sec: 1842.6835506853604
MB / sec : 0.17573199755529026
===================
  • after
$ ./run-benchmarks.sh
Running Tests for Kafka-Python
Starting benchmark for Kafka-Python producer.
Kafka-Python producer Results:
Number of Runs: 5, Number of messages: 100000, Message Size: 100 bytes.
Average Time for 100000 messages: 5.166848373413086 seconds.
Messages / sec: 19354.15804237015
MB / sec : 1.8457563440675877
===================
Running Tests for AIOKafka
/root/workspace/news/kafka-client-benchmarks/aiokafka-benchmark.py:40: DeprecationWarning: The loop argument is deprecated since 0.7.1, and scheduled for removal in 0.9.0
  producer = AIOKafkaProducer(loop=loop, **producer_config)
kafka parition size: 6056
Starting benchmark for AIOKafka Producer.
AIOKafka Producer Results:
Number of Runs: 5, Number of messages: 100000, Message Size: 100 bytes.
Average Time for 100000 messages: 3.341134214401245 seconds.
Messages / sec: 29929.95599188185
MB / sec : 2.854343032062707
===================

As for aiokafka producer, the performance is improved significantly.
Messages / sec: 1842 -> 29929
MB / sec : 0.1757 -> 2.854

Checklist

  • I think the code is well written
  • Unit tests for the changes exist (I do not modify any interface)
  • Documentation reflects the changes (same as above)
  • 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.

@uchiiii uchiiii marked this pull request as ready for review November 16, 2023 01:05
@uchiiii
Copy link
Author

uchiiii commented Nov 17, 2023

This workflow requires approval from a maintainer.

@ods Sorry to bother you, but could you approve running workflow on this PR?

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (88c6945) 84.21% compared to head (6178bcc) 84.18%.
Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #942      +/-   ##
==========================================
- Coverage   84.21%   84.18%   -0.03%     
==========================================
  Files          87       87              
  Lines       10085    10088       +3     
==========================================
  Hits         8493     8493              
- Misses       1592     1595       +3     
Flag Coverage Δ
cext 79.24% <100.00%> (-0.01%) ⬇️
integration 84.16% <100.00%> (-0.03%) ⬇️
purepy 83.94% <100.00%> (-0.03%) ⬇️
unit 51.56% <66.66%> (+0.01%) ⬆️

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.

@uchiiii
Copy link
Author

uchiiii commented Nov 22, 2023

Thank you for the approval for running workflows. I think the failed codecov is not an issue. Let me know if there is something to do.

@ods
Copy link
Collaborator

ods commented Nov 26, 2023

@uchiiii, Thank you for your submission. The solution here just moves calculation from one place to another. This improves performance in one place and may (or may not) worsen it in others. Actually, this calculations are not needed in send at all, so I decided to proceed other way. Please review pr #946, does it solve your problem?

@uchiiii uchiiii closed this Nov 27, 2023
@uchiiii uchiiii deleted the modify_cluster_metadata branch November 27, 2023 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

performance degradation of producer when having many topics
2 participants