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

Port KafkaSinkSingle to use try_recv instead of recv #1610

Open
rukai opened this issue May 7, 2024 · 0 comments
Open

Port KafkaSinkSingle to use try_recv instead of recv #1610

rukai opened this issue May 7, 2024 · 0 comments
Labels
good first issue Good for newcomers

Comments

@rukai
Copy link
Member

rukai commented May 7, 2024

prereq: #1609

This is part of #1507.
Specifically the checkbox swap single sinks to use SinkConnection::try_recv instead of SinkConnection::recv

Kafka has two sink transforms: KafkaSinkSingle and KafkaSinkCluster
KafkaSinkCluster has already been ported to use SinkConnection::try_recv instead of SinkConnection::recv.
But we still need to port KafkaSinkSingle which is the target of this issue.

  • SinkConnection::recv is an async method that asynchronously waits until at least 1 new response has arrived before returning.
  • SinkConnection::try_recv is a non-async method that immediately returns 0 or more responses.

recv introduces huge delays because it does not return until the server can respond to the request which will be in the milliseconds, which is an eternity for a proxy.
During this time shotover cannot process any new requests coming in from the client on this connection.

Previously we had to use the async wait in recv to avoid spin waiting.
But due to recent improvements to shotover internals we can now return without waiting for all responses, so it is much more efficient to use try_recv instead.

Since we expect performance improvements from this PR, appropriate windsock benchmarks should be run to demonstrate the change actually improves performance.
Take a screenshot and post it in the PR.

@rukai rukai added the good first issue Good for newcomers label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

1 participant