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

Make sure other tasks are processed with tasklists that contain both bad xfrs (that cause reload to exit) and other tasks #332

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

wtoorop
Copy link
Member

@wtoorop wtoorop commented May 25, 2024

No description provided.

This test is to make sure that the effects of other tasks are effectuated when
they are processed together with other tasks in a single tasklist.  The test
does a reload for a single bad update (from xfr) together with an addzone in a
single tasklist. The addzone should be effectuated.
So that the backup main has all other tasks processed when one of the transfers
fails (causing an exit)
Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

The code looks okay. I worry that the tasks processed by the old-main process take a long time, and that old-main is not responsive during that time. For tasks for checkzonefiles or writezonefiles, that also seems reasonable.

@wtoorop
Copy link
Member Author

wtoorop commented Aug 30, 2024

The code looks okay. I worry that the tasks processed by the old-main process take a long time, and that old-main is not responsive during that time. For tasks for checkzonefiles or writezonefiles, that also seems reasonable.

Thanks @wcawijngaards commit 0c4e275 addresses this issue you raised of notifies not being passed when non transfer tasks are being processed (and it works), although it's hard to write a unit test for this (but I've tested manually with a strategically placed sleep). If you can review again and approve (perhaps only the last commit), then I can merge this as solution for the netnod case.

server.c Outdated Show resolved Hide resolved
Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Okay that looks nice and the server stays responsive to notifies. It creates another process, but it seems to also solve the responsiveness problem.

Co-authored-by: Wouter Wijngaards <[email protected]>
Copy link
Contributor

@k0ekk0ek k0ekk0ek left a comment

Choose a reason for hiding this comment

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

I personally dislike the additional fork. IMHO it would be better to remove the NOTIFY proxy code from the main process and just let the serve child talk directly to xfrd, though there is probably a reason why it is the way it is too.

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.

3 participants