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

get-remote-ring's "other" report should only have two items. #5261

Merged
merged 1 commit into from Apr 24, 2017

Conversation

Projects
None yet
4 participants
@clokep
Contributor

clokep commented Apr 23, 2017

Short description

Requesting a remote ring via the /jsonstat endpoint returns tuples of two items: (count, IP address), except the last value which has three items: (count, "", ""). I believe this is a copy and paste error from get-query-ring section of this file. (Compare to the get-remote-ring section.)

Checklist

I have:

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

This comment has been minimized.

Show comment
Hide comment
@clokep

clokep Apr 23, 2017

Contributor

For a little context, we had discussed this on IRC ~a week ago:

12:58:58 PM - clokep: Hello! I have a question about the /jsonstats API...which I think is undocumented, but we're using it. ;) It seems that the remote-ring query returns a list of tuples of 2 (count, IP addr), but then there's sometimes an additional entry at the bottom which is (unaccounted for count, "", ""): https://github.com/PowerDNS/pdns/blob/rel/auth-4.0.x/pdns/ws-recursor.cc#L528
12:59:08 PM - clokep: Is it a bug that this has 3 entries?
1:01:07 PM - cmouse: clokep: (count,ip,addr) seems like tuple of three
1:06:17 PM - clokep: cmouse: Where do you see that?
1:06:23 PM - clokep: Line 521 looks like only two items in the array.
1:06:28 PM - clokep: -q.first, q.second.toString()
1:06:38 PM - clokep: https://github.com/PowerDNS/pdns/blob/rel/auth-4.0.x/pdns/ws-recursor.cc#L521
1:06:49 PM - clokep: (Also...the negatives in that code confuse me, but that's mostly unrelated. :))
1:06:58 PM - Habbie: i'm inclined to agree 528 should be shorter
1:08:49 PM - clokep: Should I file an issue, I (or one of my coworkers) can probably provide a PR (not sure about how quickly -- busy day here for me at least).
1:08:57 PM - Habbie: either is fine
1:09:02 PM - Habbie: and given that nothing is on fire, no rush
1:09:30 PM - clokep: Sounds good! :)
1:09:46 PM - clokep: Thanks!
1:09:51 PM - Habbie: thank you :)

After looking at it again it looks like a copy and paste error.

Unfortunately I've been unable to get pdns to compile on a completely clean Ubuntu 14.04 or 16.04. Sorry for the inconvenience. I'm more than happy to get on IRC at some point and see if we can fix my environment!

Contributor

clokep commented Apr 23, 2017

For a little context, we had discussed this on IRC ~a week ago:

12:58:58 PM - clokep: Hello! I have a question about the /jsonstats API...which I think is undocumented, but we're using it. ;) It seems that the remote-ring query returns a list of tuples of 2 (count, IP addr), but then there's sometimes an additional entry at the bottom which is (unaccounted for count, "", ""): https://github.com/PowerDNS/pdns/blob/rel/auth-4.0.x/pdns/ws-recursor.cc#L528
12:59:08 PM - clokep: Is it a bug that this has 3 entries?
1:01:07 PM - cmouse: clokep: (count,ip,addr) seems like tuple of three
1:06:17 PM - clokep: cmouse: Where do you see that?
1:06:23 PM - clokep: Line 521 looks like only two items in the array.
1:06:28 PM - clokep: -q.first, q.second.toString()
1:06:38 PM - clokep: https://github.com/PowerDNS/pdns/blob/rel/auth-4.0.x/pdns/ws-recursor.cc#L521
1:06:49 PM - clokep: (Also...the negatives in that code confuse me, but that's mostly unrelated. :))
1:06:58 PM - Habbie: i'm inclined to agree 528 should be shorter
1:08:49 PM - clokep: Should I file an issue, I (or one of my coworkers) can probably provide a PR (not sure about how quickly -- busy day here for me at least).
1:08:57 PM - Habbie: either is fine
1:09:02 PM - Habbie: and given that nothing is on fire, no rush
1:09:30 PM - clokep: Sounds good! :)
1:09:46 PM - clokep: Thanks!
1:09:51 PM - Habbie: thank you :)

After looking at it again it looks like a copy and paste error.

Unfortunately I've been unable to get pdns to compile on a completely clean Ubuntu 14.04 or 16.04. Sorry for the inconvenience. I'm more than happy to get on IRC at some point and see if we can fix my environment!

@rgacogne rgacogne added this to the rec-4.1.0 milestone Apr 24, 2017

@rgacogne

The fix looks good to me, and the corresponding JS code only uses two items.

@Habbie Habbie merged commit 73afa3b into PowerDNS:master Apr 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@clokep clokep deleted the percipient:json-stat-fix branch Apr 24, 2017

@clokep

This comment has been minimized.

Show comment
Hide comment
@clokep

clokep Apr 24, 2017

Contributor

@rgacogne Thanks for merging (and checking the JS code!)

Contributor

clokep commented Apr 24, 2017

@rgacogne Thanks for merging (and checking the JS code!)

rgacogne added a commit to rgacogne/pdns that referenced this pull request Nov 13, 2017

@rgacogne rgacogne referenced this pull request Nov 13, 2017

Merged

Recursor 4.0.7 backports #5952

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