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

[native] Reduce numer of http requests for Exchange #23097

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

arhimondr
Copy link
Member

Description

Reduce number of HTTP requests issued by exchange protocol

Motivation and Context

With high number of partitions the amount of data exchanged in a single round trip can be quite low (~50kb). This PR reduces number of explicit acknowledge calls as well as increases the long pool delay to avoid sending unnecessary pings when the buffers are empty (for example of not yet active stages).

Impact

Improves cpu efficiency at a fully utilized cluster by ~5%.

Test Plan

CI

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

No need to send an explicit ack when PrestoExchangeSource is active. A
subsequent getData request is expected to acknowledge previously
received data automatically
To accommodate long pool delay increase in Velox (2s -> 10s). The request
timeout has to be higher than the long pool delay to let the remote
server properly respond before terminating a connection.
@arhimondr arhimondr requested a review from a team as a code owner June 27, 2024 18:59
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@arhimondr thanks!

@amitkdutta amitkdutta merged commit 0a17a67 into prestodb:master Jun 28, 2024
59 checks passed
@majetideepak
Copy link
Collaborator

Let's try to advance Velox in a separate PR when possible.

@majetideepak
Copy link
Collaborator

My bad! I just noticed it is in a separate commit. This works!

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.

None yet

4 participants