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

Small improvements to the Producer #1272

Merged
merged 3 commits into from
Jul 14, 2024
Merged

Small improvements to the Producer #1272

merged 3 commits into from
Jul 14, 2024

Conversation

erikvanoosten
Copy link
Collaborator

By using ZIO.async, we no longer need a reference to the zio runtime, nor do we need the exec trickery anymore.

Copy link
Collaborator

@svroonland svroonland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better this way. I think this code stems from very early ZIO days, possibly there was some reason back then not to use ZIO.async, but I can't think of any.

@erikvanoosten erikvanoosten merged commit 9a60ccd into master Jul 14, 2024
14 checks passed
@erikvanoosten erikvanoosten deleted the producer-cleanup branch July 14, 2024 10:01
svroonland pushed a commit that referenced this pull request Aug 10, 2024
By using ZIO.async, we no longer need a reference to the zio runtime,
nor do we need the `exec` trickery anymore.
erikvanoosten pushed a commit that referenced this pull request Aug 21, 2024
During running services with the new version of library 2.8.1, I noticed
huge increase in messages production time to kafka.
Some quick rough tests shoving me around 40x-100x times increase in
amount of time taken to `produceChunk` on even 1-10 records chunks
comparing to version 2.8.0 (or also to version 2.8.1 with the changes in
this MR). Please, note, it is not a proper benchmarks. Also, to note, Im
using Scala version 3.3.1.

This MR just reverts two MRs updating `ProducersendFromQueue`
implementation:
 - #1272
 - #1285
 
Not sure if it's possible to revert two MRs at a time (with a single one
for revert), so created this one.
 
I haven't researched yet which exact change/changes are causing such
performance degradation.
 I would suggest the next steps:
  - confirm the problem exists
- reverting to the previous implementation (the one from 2.8.0/this MR)
  - release fixed version (to allow users have a nicely working version)
- investigate & fix problem from the
#1272 and/or
#1285

It is only suggestions on the approach, feel free to ignore them.
Also, feel free to modify/ignore this MR and treat it as an issue.
erikvanoosten added a commit that referenced this pull request Aug 24, 2024
Refactoring of the producer so that it handles errors per record.

These changes were part of 2.8.1 (via #1285 and #1272) but were reverted in 2.8.2 (via #1304) after reports of performance regression. This is a retry to make it easier to run the benchmarks and make further improvements.

This reverts commit 3393fbf.
erikvanoosten added a commit that referenced this pull request Aug 24, 2024
Refactoring of the producer so that it handles errors per record.

These changes were part of 2.8.1 (via #1285 and #1272) but were reverted in 2.8.2 (via #1304) after reports of performance regression. This is a retry to make it easier to run the benchmarks and make further improvements.

This reverts commit 3393fbf.
erikvanoosten added a commit that referenced this pull request Aug 25, 2024
Refactoring of the producer so that it handles errors per record.

These changes were part of 2.8.1 (via #1285 and #1272) but were reverted in 2.8.2 (via #1304) after reports of performance regression. This is a retry to make it easier to run the benchmarks and make further improvements.

This reverts commit 3393fbf.
erikvanoosten added a commit that referenced this pull request Aug 26, 2024
Refactoring of the producer so that it handles errors per record.

These changes were part of 2.8.1 (via #1285 and #1272) but were reverted in 2.8.2 (via #1304) after reports of performance regression. This is a retry to make it easier to run the benchmarks and make further improvements.

This reverts commit 3393fbf.
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.

2 participants