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

Producer alternative send chunk #1284

Closed
wants to merge 5 commits into from

Conversation

domdorn
Copy link

@domdorn domdorn commented Jul 16, 2024

for discussion: alternative implementation of the sendChunk method..

@erikvanoosten
Copy link
Collaborator

@domdorn Nice idea! This will work definitely better than the proposal in #1283. Is it okay if I take this and work it out further?

@domdorn
Copy link
Author

domdorn commented Jul 16, 2024 via email

@erikvanoosten
Copy link
Collaborator

erikvanoosten commented Jul 16, 2024

Sorry, this won't work either. We cannot afford to wait for the broker to acknowledge every record, it would be super slow!

@erikvanoosten
Copy link
Collaborator

Sorry, this won't work either. We cannot afford to wait for the broker to acknowledge every record, it would be super slow!

Oops, again sorry. That comment doesn't apply to your code but a refactoring I did 😊.
There are still 2 other problems with this proposal though:

  • it uses zio.fromJavaFuture which creates 1 thread per record, the thread is blocked until the record is acknowledged
  • the records are no longer send in order

@domdorn domdorn force-pushed the producer_alternative_sendChunk branch from 136aed2 to 2752d1e Compare July 17, 2024 15:59
@domdorn domdorn force-pushed the producer_alternative_sendChunk branch from 2752d1e to 62f5689 Compare July 17, 2024 16:01
@domdorn
Copy link
Author

domdorn commented Jul 18, 2024

closed in favour of #1286 (although I liked my ZIO.async stuff..)

@domdorn domdorn closed this Jul 18, 2024
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