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

dnsdist: Better handling of multiple carbon servers #12424

Merged
merged 2 commits into from
Jan 24, 2023

Conversation

rgacogne
Copy link
Member

Short description

This PR implements a refactoring of the Carbon handling in dnsdist. Each remote endpoint is now handled by its own thread, as to prevent a slow/unreachable server from impacting others.
It also implements an exponential back-off when a server cannot be reached properly, as to not overload a suffering endpoint.

Hopefully closes #10517 and #11216.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)


void carbonDumpThread()
static bool doOneCarbonExport(const Carbon::Endpoint& endpoint)

Check warning

Code scanning / CodeQL

Poorly documented large function

Poorly documented function: fewer than 2% comments for a function of 227 lines.
pdns/dnsdist-carbon.cc Fixed Show resolved Hide resolved
@rgacogne rgacogne marked this pull request as ready for review January 18, 2023 15:53
@rgacogne rgacogne requested a review from omoerbeek January 18, 2023 15:53
Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

A few nits

pdns/dnsdist-carbon.cc Fixed Show resolved Hide resolved
pdns/dnsdist-carbon.cc Outdated Show resolved Hide resolved
pdns/dnsdist-carbon.cc Show resolved Hide resolved
- We now explicitly convert to double, making sure that we will not
  overflow by restricting the value of the counter
- Clear the endpoints list when the carbon threads are started, to
  make clear we do not need them anymore
- Move the endpoints passed to the carbon threads, to make static
  analysis tools happy.
@rgacogne
Copy link
Member Author

I pushed a new commit:

  • We now explicitly convert to double, making sure that we will not overflow by restricting the value of the counter
  • Clear the endpoints list when the carbon threads are started, to make clear we do not need them anymore
  • Move the endpoints passed to the carbon threads, to make static analysis tools happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dnsdist: Carbon intervals are slightly off when exporting to multiple servers and one is unreachable
2 participants