Skip to content

feat(object_store): Override DNS Resolution to Randomize IP Selection#7123

Merged
tustvold merged 2 commits intoapache:mainfrom
crepererum:crepererum/issue7117
Feb 12, 2025
Merged

feat(object_store): Override DNS Resolution to Randomize IP Selection#7123
tustvold merged 2 commits intoapache:mainfrom
crepererum:crepererum/issue7117

Conversation

@crepererum
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes apache/arrow-rs-object-store#19.

Rationale for this change

See apache/arrow-rs-object-store#19.

What changes are included in this PR?

Opt-out, randomizing resolver config.

Are there any user-facing changes?

Load is now spread across multiple servers.

Comment thread object_store/Cargo.toml Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that these dependencies aren't technically new, they were already indirect dependencies. They are mostly needed because the relevant reqwest types aren't public. I'll try to fix that in seanmonstar/reqwest#2549 , but I don't think we need to block this PR here on that upstream change.

Comment thread object_store/src/client/dns.rs Outdated

use std::str::FromStr;

use hyper_util::client::legacy::connect::dns::{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the legacy part a concern? Do we need to use this or could we just inline whatever it is doing if it isn't complex?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's used by reqwest as well. I think it mostly means that his is the hyper 0.x legacy behavior that you should eventually migrate away from -- which we could do eventually (see #7123 (comment) ).

However you're also right that we could just perform the std call ourselves, will try that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done, and the code is IMHO actually rather simple

@crepererum crepererum requested a review from tustvold February 12, 2025 12:26
@tustvold tustvold added the api-change Changes to the arrow API label Feb 12, 2025
Copy link
Copy Markdown
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

I've marked this as a breaking change, just to highlight it in the changelog. The next object_store release is breaking anyway

@tustvold tustvold merged commit d3a875f into apache:main Feb 12, 2025
crepererum added a commit to influxdata/arrow-rs that referenced this pull request Feb 12, 2025
Makes apache#7123 compatible w/ an
older version of `rand`.
@tustvold
Copy link
Copy Markdown
Contributor

FWIW this is actually a breaking change, as reqwest has a feature flag that can enable hickory DNS, which this will override

@tustvold tustvold changed the title feat(object_store): random IP address selection feat(object_store): Override DNS Resolution to Randomize IP Selection Feb 16, 2025
alamb pushed a commit to alamb/arrow-rs that referenced this pull request Mar 20, 2025
* feat(object_store): random IP address selection

Closes #7117.

* refactor: directly call stdlib w/o hyper-util
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-change Changes to the arrow API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Object Store: S3 IP address selection is biased

2 participants