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

Missing RR implementation & advanced usage #8

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@attozk

attozk commented Oct 2, 2014

  • Added support for MX, TXT, PTR, SOA, ANY, NS, AAAA records
  • Added additional & authority message parsers & dumper (includes RobinvdVleuten's reactphp/react#272)
  • Fixed TCP retry parser & dumper. In TCP first two octets describe the length of data
  • Added explain method to Message, Query, Record for detailed information in a dig like format.
  • Added examples
  • Message->questions now stores array of Query objects instead of array of array

attozk added some commits Oct 2, 2014

- Added lookup & reverse methods to resolver
- Added debugging methods (dig like output) and HumanParser
- Added support for MX, NS, TXT, PTR, SOA records
- Added example scripts
Update Parser.php
parser to not chock on types not implemented yet
- Added support (and examples) for AAAA
- Added transport info at Message level to parse/binarydump TCP/UDP properly. In TCP first two octets describes the length of data
-- Added execution time calculation to Message and pretty EXPLAIN() with nameserver and data size info
- Added CODE type to Record for Humans
- Added binarydumper for Answers, Authority & Additional (this now covers RobinvdVleuten'shttps://github.com/reactphp/react/pull/272)
@attozk

This comment has been minimized.

Show comment
Hide comment
@attozk

attozk commented Oct 12, 2014

@igorw @clue ping?

@robinvdvleuten

This comment has been minimized.

Show comment
Hide comment
@robinvdvleuten

robinvdvleuten Oct 12, 2014

wow impressive work @attozk 👍

robinvdvleuten commented Oct 12, 2014

wow impressive work @attozk 👍

@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Oct 19, 2014

Contributor

@attozk wow, thanks for this pull request! reviewing now. ;-)

Contributor

igorw commented Oct 19, 2014

@attozk wow, thanks for this pull request! reviewing now. ;-)

Show outdated Hide outdated src/Model/Message.php
@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Oct 19, 2014

Contributor

This PR would be a lot easier to review and merge if it were broken down into separate smaller PRs for the individual features. I have started reviewing and reformatting it. Might submit them back to you as we go along.

Contributor

igorw commented Oct 19, 2014

This PR would be a lot easier to review and merge if it were broken down into separate smaller PRs for the individual features. I have started reviewing and reformatting it. Might submit them back to you as we go along.

@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Oct 19, 2014

Contributor

First PR here attozk#1

Contributor

igorw commented Oct 19, 2014

First PR here attozk#1

@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Oct 19, 2014

Contributor

Really awesome to support more record types, IPv6 and fix the missing length prefix for TCP! Thanks for the hard work!

Contributor

igorw commented Oct 19, 2014

Really awesome to support more record types, IPv6 and fix the missing length prefix for TCP! Thanks for the hard work!

@attozk

This comment has been minimized.

Show comment
Hide comment
@attozk

attozk Oct 20, 2014

@igorw I should have been more careful with formatting (I blame my IDE for that :p). Will work with you on attozk#1 for getting this merged.

But thank you for leaving enough examples in the code which made it easier to expand it further.

I think you might be interested in knowing that a react-DNS server is just as fast as PDNS (both with mysql backend)

attozk commented Oct 20, 2014

@igorw I should have been more careful with formatting (I blame my IDE for that :p). Will work with you on attozk#1 for getting this merged.

But thank you for leaving enough examples in the code which made it easier to expand it further.

I think you might be interested in knowing that a react-DNS server is just as fast as PDNS (both with mysql backend)

@attozk

This comment has been minimized.

Show comment
Hide comment
@attozk

attozk Oct 21, 2014

@igorw Updated per your initial review:

  • HumanParse is stateless now
  • Removed time tracking from Message to MessageMata ($message->meta). I still have $transport & $nameserver as part of Message. IMO it makes sense to keep them Message.

Thoughts?

attozk commented Oct 21, 2014

@igorw Updated per your initial review:

  • HumanParse is stateless now
  • Removed time tracking from Message to MessageMata ($message->meta). I still have $transport & $nameserver as part of Message. IMO it makes sense to keep them Message.

Thoughts?

@clue clue referenced this pull request Oct 23, 2014

Closed

Querying via TCP broken #11

class HumanParser
{
private static $arrTYPE = array('A' => 1,

This comment has been minimized.

@igorw

igorw Nov 16, 2014

Contributor

please no hungarian notation, just called it "types" maybe?

@igorw

igorw Nov 16, 2014

Contributor

please no hungarian notation, just called it "types" maybe?

'AAAA' => 28,
'ANY' => 255);
private static $arrTYPEHuman = array(1 => 'A',

This comment has been minimized.

@igorw

igorw Nov 16, 2014

Contributor

this can be computed from the other array with an array_flip()

@igorw

igorw Nov 16, 2014

Contributor

this can be computed from the other array with an array_flip()

This comment has been minimized.

@attozk

attozk Nov 19, 2014

Trying to keep it stateless so not sure

@attozk

attozk Nov 19, 2014

Trying to keep it stateless so not sure

Show outdated Hide outdated src/Protocol/Parser.php
Show outdated Hide outdated src/Protocol/Parser.php
public function parseAnswer(Message $message)
{
return $this->parseRecord($message, 'answer');
}

This comment has been minimized.

@igorw

igorw Nov 16, 2014

Contributor

might as well get rid of this method

@igorw

igorw Nov 16, 2014

Contributor

might as well get rid of this method

$queryData = $this->dumper->toBinary($request);
$transport = strlen($queryData) > 512 ? 'tcp' : 'udp';
$query->attempts++;

This comment has been minimized.

@igorw

igorw Nov 16, 2014

Contributor

Still not a fan of this type of mutation on data objects. It's convenient to put it there, but it's not really part of the query data.

@igorw

igorw Nov 16, 2014

Contributor

Still not a fan of this type of mutation on data objects. It's convenient to put it there, but it's not really part of the query data.

@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Nov 16, 2014

Contributor

Regarding the mutation of state. And the "metadata" being part of the message object. IMO it should be the other way round. Basically, once a query enters the system, we should create a "State" data object that holds the query and state that may change over time, such as the retries.

This way the messages are completely immutable, as they should be, and all the mutable state is in one place.

This state object is then passed through the system instead of query/response. Does that make any sense?

Contributor

igorw commented Nov 16, 2014

Regarding the mutation of state. And the "metadata" being part of the message object. IMO it should be the other way round. Basically, once a query enters the system, we should create a "State" data object that holds the query and state that may change over time, such as the retries.

This way the messages are completely immutable, as they should be, and all the mutable state is in one place.

This state object is then passed through the system instead of query/response. Does that make any sense?

@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Nov 20, 2014

Contributor

For the record, I can see merging this PR with the state mutation as is, but as a long term goal we should probably move to a more immutable/explicit model.

Contributor

igorw commented Nov 20, 2014

For the record, I can see merging this PR with the state mutation as is, but as a long term goal we should probably move to a more immutable/explicit model.

@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Nov 20, 2014

Contributor

After fixing those last few things I'd say we're good to go. Sorry for taking so long with the reviews!

Contributor

igorw commented Nov 20, 2014

After fixing those last few things I'd say we're good to go. Sorry for taking so long with the reviews!

@attozk

This comment has been minimized.

Show comment
Hide comment
@attozk

attozk Nov 20, 2014

@igorw let me take a stab at metadata first and lets merge after you review it.

attozk commented Nov 20, 2014

@igorw let me take a stab at metadata first and lets merge after you review it.

- naming convention
- strict string compression when checking for tcp mode
- added unknown for RRs not supported
- using microtime(true) for execution time
@attozk

This comment has been minimized.

Show comment
Hide comment
@attozk

attozk Jan 11, 2015

@igorw
Sorry took me while to get back to this. What are your thoughts about merging it at this point and tackling state mutation later on as you originally suggested.

attozk commented Jan 11, 2015

@igorw
Sorry took me while to get back to this. What are your thoughts about merging it at this point and tackling state mutation later on as you originally suggested.

@attozk

This comment has been minimized.

Show comment
Hide comment
@attozk

attozk commented Mar 12, 2015

@igorw , @cboden

ping?

@WyriHaximus

This comment has been minimized.

Show comment
Hide comment
@WyriHaximus

WyriHaximus May 18, 2015

Member

@clue Maybe something for you since you're also working on some more DNS PR's?

Member

WyriHaximus commented May 18, 2015

@clue Maybe something for you since you're also working on some more DNS PR's?

@clue

This comment has been minimized.

Show comment
Hide comment
@clue

clue Jun 6, 2015

Member

This PR does in fact contain quite a bit of useful code that I'd love to see included! Though I agree with @igorw this is a bit hard to review given the amount of code changed. Ideally, this would be split up into several smaller PRs.

Either way, is there anything that can be done to move this forward?

Member

clue commented Jun 6, 2015

This PR does in fact contain quite a bit of useful code that I'd love to see included! Though I agree with @igorw this is a bit hard to review given the amount of code changed. Ideally, this would be split up into several smaller PRs.

Either way, is there anything that can be done to move this forward?

@clue clue referenced this pull request Jun 6, 2015

Closed

[WIP] DNS Server Implementation #9

@attozk

This comment has been minimized.

Show comment
Hide comment
@attozk

attozk Sep 20, 2015

I would love to see this merged. Is there anything you need from me at this point @clue

attozk commented Sep 20, 2015

I would love to see this merged. Is there anything you need from me at this point @clue

@attozk attozk closed this Dec 30, 2015

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