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

chore(networking)!: Remove custom DNS resolution #2812

Merged
merged 6 commits into from
Jun 18, 2020
Merged

Conversation

ktff
Copy link
Contributor

@ktff ktff commented Jun 14, 2020

Closes #2635.

Open questions

  • If we don't have intention to have dns configurable and it's fine to not call spawn_blocking through a runtime handle, then we can further remove passing Resolver through contexts and functions, and instead construct Resolver directly at call site. EDIT: We'll keep it as it is.

cc. @lukesteensen

@ktff ktff added the domain: networking Anything related to Vector's networking label Jun 14, 2020
@ktff ktff requested a review from MOZGIII June 14, 2020 18:58
@ktff ktff self-assigned this Jun 14, 2020
@ktff ktff removed the request for review from MOZGIII June 14, 2020 18:58
@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching Cargo.lock:

  • Has at least one other team member approved the dependency changes?

This is an automatically generated QA checklist based on modified files

@ktff
Copy link
Contributor Author

ktff commented Jun 14, 2020

@Jeffail I'll remove you from review since your file is only slightly changed in tests. But you are welcome to review nevertheless.

@ktff ktff removed the request for review from Jeffail June 14, 2020 19:02
@binarylogic binarylogic changed the title chore(networking): Remove custom DNS resolution chore(networking)!: Remove custom DNS resolution Jun 14, 2020
@MOZGIII
Copy link
Contributor

MOZGIII commented Jun 15, 2020

it's fine to not call spawn_blocking through a runtime handle

spawn_blocking doesn't take a runtime handle, so it's fine

instead construct Resolver directly at call site

If we want to have it centrally configure - we definitely want it to be passed centrally. However, I'm not sure what our intent is.
To decide rationally - we have to ask ourselves what are the use cases we want to cover here? How would we use custom DNS resolvers?
Personally, and, maybe, not that rationally, I'd prefer to have an infrastructure for using custom DNS resolvers that would allow specifying either the system-configured resolver as the default or, if it's required in a particular case - use a custom resolver. I feel like this approach, regardless of the use cases, is a good balance between flexibility and complexity overhead.

@ktff
Copy link
Contributor Author

ktff commented Jun 15, 2020

spawn_blocking doesn't take a runtime handle, so it's fine

But it does require the runtime, that's why it must be called from inside of one. The tokio v0.2 that we will transition to eventually does expose a way to make this dependency explicit via a handle.

src/dns.rs Show resolved Hide resolved
Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

This is great! Very excited to see this happen.

Even though we've dropped the state, I still think it's fine to keep resolver on the context and access it that way. This means users of the API don't need to care how it's implemented and we can do things like thread an executor automatically.

@ktff
Copy link
Contributor Author

ktff commented Jun 17, 2020

Noting, do not merge this, we are getting
(signal: 11, SIGSEGV: invalid memory reference)
on MAC

@ktff
Copy link
Contributor Author

ktff commented Jun 18, 2020

The error started popping elsewhere too so it's not related to this PR.

ktff added 5 commits June 18, 2020 12:58
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
@ktff ktff merged commit 49de96d into master Jun 18, 2020
@ktff ktff deleted the ktff/simplify_dns branch June 18, 2020 12:32
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
* Remove trust-dns

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>

* Remove unecessary clone

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>

* Further simplify

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>

* Try with ipv4

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>

* Comment

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>

* Use block_on in a test

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: networking Anything related to Vector's networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removal of custom DNS resolution
3 participants