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

rec: Move carbon/webserver/control/stats handling to a separate thread #6567

Merged
merged 8 commits into from May 17, 2018

Conversation

Projects
None yet
4 participants
@rgacogne
Member

rgacogne commented May 3, 2018

Short description

This makes sure that no worker or distributor thread will get blocked while waiting for a response from another thread, for example while gathering stats or executing a command coming from the control
channel.

This PR needs a serious review, don't merge blindly!

It might also need some documentation update.

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)
Show outdated Hide outdated pdns/pdns_recursor.cc
Show outdated Hide outdated pdns/pdns_recursor.cc
@chbruyand

Maybe a word or two around the s_ThreadID constants and the t_id variable to document the big picture would be nice, but otherwise it looks really good.

// call the function ourselves, to update the ACL or domain maps for example
func();
int n = 0;
for(ThreadPipeSet& tps : g_pipes)
{
if(n++ == t_id) {

This comment has been minimized.

@chbruyand

chbruyand May 7, 2018

Member

func() may be called twice (when this is called by the distributor) ?

@chbruyand

chbruyand May 7, 2018

Member

func() may be called twice (when this is called by the distributor) ?

@zeha

zeha approved these changes May 14, 2018

@pieterlexis

This PR has been tested thoroughly in a test lab last week!

@rgacogne

This comment has been minimized.

Show comment
Hide comment
@rgacogne

rgacogne May 15, 2018

Member

I just noticed a small issue with this PR. The doStats() function is called from the houseKeeping() (except if the user requested a stats dump via USR1), and houseKeeping() is invoked every 500 iterations of the main recursor thread loop. But in the new handler thread that has no DNS query to process, 500 iterations is a very long time, that in theory could be as long as 500 times the 500ms timeout of the multiplexer.
I'll move the invocation of doStats() out of houseKeeping() so it's called every g_statisticsInterval seconds.

Member

rgacogne commented May 15, 2018

I just noticed a small issue with this PR. The doStats() function is called from the houseKeeping() (except if the user requested a stats dump via USR1), and houseKeeping() is invoked every 500 iterations of the main recursor thread loop. But in the new handler thread that has no DNS query to process, 500 iterations is a very long time, that in theory could be as long as 500 times the 500ms timeout of the multiplexer.
I'll move the invocation of doStats() out of houseKeeping() so it's called every g_statisticsInterval seconds.

rgacogne added a commit to rgacogne/pdns that referenced this pull request May 16, 2018

rgacogne added a commit that referenced this pull request May 16, 2018

rgacogne added some commits May 3, 2018

rec: Move carbon/webserver/control/stats handling to a separate thread
This makes sure that no worker or distributor thread will get blocked
while waiting for a response from another thread, for example while
gathering stats or executing a command coming from the control
channel.
rec: Don't call the broadcast function twice in the handler thread
Also add some comments to clarify how the threads work.
@rgacogne

This comment has been minimized.

Show comment
Hide comment
@rgacogne

rgacogne May 16, 2018

Member

Rebased to fix a conflict with #6566.

Member

rgacogne commented May 16, 2018

Rebased to fix a conflict with #6566.

@rgacogne rgacogne merged commit fa2acc4 into PowerDNS:master May 17, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: C/C++ No alert changes
Details
lgtm analysis: JavaScript No alert changes
Details
lgtm analysis: Python No alert changes
Details

@rgacogne rgacogne deleted the rgacogne:rec-separate-handler-thread branch May 17, 2018

@rgacogne rgacogne referenced this pull request May 18, 2018

Merged

rec: Add lua maintenance callback #6589

4 of 7 tasks complete

@Habbie Habbie referenced this pull request Jul 26, 2018

Merged

rec: load Lua scripts only in worker threads #6812

3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment