Opened 9 years ago

Closed 9 years ago

#439 closed task (complete)

Implement normal query processing

Reported by: zzchen_pku Owned by: zzchen_pku
Priority: medium Milestone:
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is an update to #418. It should take MemoryDataSrc? (instead of a zone table). Also, if zone->find() is successful, add the returned RRset to the answer section of the response. As part of this task, we'd also update !AuthSrvImpl::processNormalQuery() so that if a memory data source is configured call the separate Query::process() method instead of !DataSRc::doQuery().

Subtickets

Change History (7)

comment:1 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jinmei
  • Status changed from new to reviewing

Currently, MemoryDataSrc? isn't a derived class of AbstractDataSrc?, so I defined a new variable in AuthSrvImpl?. I also add a config data item to enable MemoryDataSrc?, if MemoryDataSrc? is diabled, SQLite data source will be used.

comment:2 follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

A few initial things:

  • the important part (successful zone->find()) isn't tested. even if we don't yet have a complete find() implementation of the real MemoryZone? class, we can test the case using a mock Zone class for testing (and we should do so). Such a mock class would simply hardcode the results, e.g, returning NXDOMAIN for nxdomain.example.com; returning CNAME for cname.example.com, etc. In fact, even if we complete the MemoryZone::find() work, it would still make sense to test the Query behavior using a mock because we can then omit loading a zone file and won't be bothered with possible bugs in the MemoryZone? implementation.
  • as a related note, we should primarily write tests for the Query class in query_unittest.cc, not in auth_srv.cc. Then we don't even have to create a server object for the testing. In general, a class should be tested with minimal dependencies. we'll still need a test for the AuthSrv? to check if the new query logic is used when the memory data source is configured, but that test can (should) be much simpler in terms of query processing.
  • regarding pimpl: while I'm generally a fan (or advocate) of pimpl, in this particular case we might want to avoid that. The Query class would be instantiated for every query in a performance sensitive path, and the construction/destruction overhead due to new/delete with the pimpl may be substantial. This concern may still be a matter of premature optimization, so if there's a reason that can make the code (e.g.) more robust, I'm okay with keeping the pimpl approach. In that case please leave comments about performance consideration.
  • this code is NO-OP:
                    isc_throw(Unexpected,
                              "Zone::find return unexpected result.");
    
    Note: we should have been able to avoid this type of trivial error with TDD.

BTW: I may not be able to take on further review once these points are addressed as I've already committed to several other things (related to BIND 10 development). Maybe you want to contact Michal if he's available.

comment:3 in reply to: ↑ 2 Changed 9 years ago by zzchen_pku

Replying to jinmei:

A few initial things:

  • the important part (successful zone->find()) isn't tested. even if we don't yet have a complete find() implementation of the real MemoryZone? class, we can test the case using a mock Zone class for testing (and we should do so). Such a mock class would simply hardcode the results, e.g, returning NXDOMAIN for nxdomain.example.com; returning CNAME for cname.example.com, etc. In fact, even if we complete the MemoryZone::find() work, it would still make sense to test the Query behavior using a mock because we can then omit loading a zone file and won't be bothered with possible bugs in the MemoryZone? implementation.

Updated, add a MockZone? class for testing.

  • as a related note, we should primarily write tests for the Query class in query_unittest.cc, not in auth_srv.cc. Then we don't even have to create a server object for the testing. In general, a class should be tested with minimal dependencies. we'll still need a test for the AuthSrv? to check if the new query logic is used when the memory data source is configured, but that test can (should) be much simpler in terms of query processing.

Done.

  • regarding pimpl: while I'm generally a fan (or advocate) of pimpl, in this particular case we might want to avoid that. The Query class would be instantiated for every query in a performance sensitive path, and the construction/destruction overhead due to new/delete with the pimpl may be substantial. This concern may still be a matter of premature optimization, so if there's a reason that can make the code (e.g.) more robust, I'm okay with keeping the pimpl approach. In that case please leave comments about performance consideration.

Agree, updated.

  • this code is NO-OP:
                    isc_throw(Unexpected,
                              "Zone::find return unexpected result.");
    
    Note: we should have been able to avoid this type of trivial error with TDD.

Removed.

BTW: I may not be able to take on further review once these points are addressed as I've already committed to several other things (related to BIND 10 development). Maybe you want to contact Michal if he's available.

Okay, thank you for your comments.

comment:4 follow-up: Changed 9 years ago by vorner

Just one thing I noticed. The MockZone? class looks little bit overcomplicated. I see no need for pimpl there, as it is technique used to hide implementation details away from interface. We don't really hide it, it is in the same .cc file and we have no need to hide it, since the class is not part of any public API.

As well, providing it with origin and class makes little sense. It will ever be used only as example.com. zone in the IN class, so instead of having the constructor parameters, passing them to parameters of the MockZoneImpl? and storing them, hardcoding the answers to getOrigin and getClass seems better (and not remembering any state and not having constructor/destructors).

comment:5 in reply to: ↑ 4 Changed 9 years ago by zzchen_pku

Replying to vorner:

Just one thing I noticed. The MockZone? class looks little bit overcomplicated. I see no need for pimpl there, as it is technique used to hide implementation details away from interface. We don't really hide it, it is in the same .cc file and we have no need to hide it, since the class is not part of any public API.

As well, providing it with origin and class makes little sense. It will ever be used only as example.com. zone in the IN class, so instead of having the constructor parameters, passing them to parameters of the MockZoneImpl? and storing them, hardcoding the answers to getOrigin and getClass seems better (and not remembering any state and not having constructor/destructors).

Done, simplified the MockZone? Class.

comment:6 follow-up: Changed 9 years ago by vorner

I did one minor change (put the MockZone? to anonymous zone, as it is not needed in the isc::datasrc). If you don't mind, please, merge.

comment:7 in reply to: ↑ 6 Changed 9 years ago by zzchen_pku

  • Resolution set to complete
  • Status changed from reviewing to closed

Replying to vorner:

I did one minor change (put the MockZone? to anonymous zone, as it is not needed in the isc::datasrc). If you don't mind, please, merge.

Okay, I have merged it with config knob. Closing.
Thank you for review.

Note: See TracTickets for help on using tickets.