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

Use protozero for Protocol Buffer operations in dnsdist, and dnstap/outgoing for the rec #9843

Merged
merged 13 commits into from Jan 6, 2021

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Dec 9, 2020

Short description

This PR:

  • switches from libprotobuf to protozero to generate Protocol Buffer and dnstap messages in dnsdist ;
  • same for outgoing Protocol Buffer and dnstap messages in the recursor ;
  • same for dnscap2protobuf in the auth ;
  • removes the dependencies on libprotobuf and proto-c ;
  • removes the --with-protobuf option since protozero is always available, removing a ton of #ifdefs.

Using protozero in dnsdist results in no new allocation when a message in generated, against ~8 of them before.

Closes #9780.
Closes #9781.

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)

@rgacogne
Copy link
Member Author

Great, it looks like applying our formatting script locally gives a different result than the one expected in Circle CI..

pdns/lwres.cc Outdated Show resolved Hide resolved
pdns/lwres.cc Outdated Show resolved Hide resolved
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.

This is a big one! I did not read everything, but in general it looks very good and I'm very happy with it.

I tested dnstap, regular protobuf and outgoing protobuf in rec (and noted one issue in the dnstap code as already mentioned).

I also reviewed the recursor changes in detail, since they are a bit tricky with adding to a partially constructed message. That all looks fine and tests ok.

Two things remain:

  • It really could use more extensive regression tests.
  • Also, a few symbolic constants or enums would make the code easier to read and might even have prevented the 3/4 mixup.

@rgacogne
Copy link
Member Author

It really could use more extensive regression tests.

It turns out that the current regression tests are quite good, and would have caught the issue if they were run in our CI.. Which #9836 will solve. And apparently I forgot to run them locally, as I was too focused on the protobuf ones.

Also, a few symbolic constants or enums would make the code easier to read and might even have prevented the 3/4 mixup.

I'll push a couple commits doing just that in a few moments.

Thanks for the review, Otto, much appreciated!

@rgacogne rgacogne force-pushed the dnsdist-protozero branch 2 times, most recently from d62f6a5 to fc65533 Compare December 15, 2020 09:00
@rgacogne
Copy link
Member Author

Otto, do you want to take a look at the two latest commits before I merge this PR?

There is no rush, I won't merge it before auth-4.4.0 since I'm afraid the removal of libprotobuf might cause a few build issues here and there.

@rgacogne
Copy link
Member Author

rgacogne commented Jan 5, 2021

Rebased to fix a conflict introduced by the incbin removal.

@rgacogne rgacogne merged commit d49bc4b into PowerDNS:master Jan 6, 2021
@rgacogne rgacogne deleted the dnsdist-protozero branch January 6, 2021 10:29
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.

rec: more protozero dnsdist: Ponder switching to protozero
2 participants