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

dns-discovery-netty: Add support for DNS hedging requests #2918

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bryce-anderson
Copy link
Contributor

@bryce-anderson bryce-anderson commented May 10, 2024

Motivation:

DNS often sends requests over UDP and UDP transport is
not durable, meaning that it needs to be expected that
packets will get lost. This will translate into DNS timeouts.
This is unfortunate because timeouts can be very long, on
the order of seconds, while DNS resolutions can be much
faster than this.

Modifications:

Add support for hedging. After a period of time we can
send a second request that can race with the first to hedge
against lost packets. To make sure we don't end up with
a flood we use a token bucket to limit how often we can
send hedging requests.

@bryce-anderson bryce-anderson changed the title Bl anderson/dns hedging requests dns-discovery-netty: Add support for DNS hedging requests May 10, 2024
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

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

Overall direction LGTM. Maybe let's postpone it until after the release is cut to give it more time to bake?

boolean withdraw();
}

// TODO: both these implementations are un-synchronized and rely on netty using only a single event loop.
Copy link
Member

Choose a reason for hiding this comment

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

We always use only a single event loop and this is ensured by inEventLoop checks across DefaultDnsClient

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

2 participants