BIND 10 #221: review: Refactoring auth server and merge the axfr and notify logic into it

[BIND 10 Development]

#221: review: Refactoring auth server and merge the axfr and notify logic into it
------------------------------+---------------------------------------------
      Reporter:  hanfeng      |        Owner:  jinmei                     
          Type:  enhancement  |       Status:  reviewing                  
      Priority:  major        |    Milestone:  06. 4th Incremental Release
     Component:  b10-auth     |   Resolution:                             
      Keywords:               |    Sensitive:  0                          
Estimatedhours:               |        Hours:                             
      Billable:  1            |   Totalhours:                             
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by jinmei):

 Replying to [comment:25 each]:
 > I don't see anything particularly wrong here, but I do have a few nits
 and some questions about design decisions, see below.
 >
 > It's generally okay, and I expect any changes you make in response to
 the comments below to be fairly insignificant.  I'll leave it to your
 judgment whether to go ahead and commit after you've made such changes, or
 to seek further review.
 >
 Thanks for the detailed review.  I'm going to incorporate trivial changes
 and merge the branch to trunk anyway because other important features such
 as secondary manager depends on it.  We can create a separate follow up
 tickets for any remaining open issues.

 I'll give this ticket back to you, keeping it open just in case.  If you
 think this particular ticket is done, please simply close it; if you want
 to continue discussions on some of the points below within this ticket,
 please do so; if you want to discuss the points in a seprate ticket,
 please close this one and open new one(s).

 > Index: src/lib/xfr/xfrout_client.h
 >
 > Minor nit 1:  In some other abstract base classes (e.g.
 !AbstractDataSrc, AbstractRRset) you've put in a boilerplate comment
 explaining why the constructor is declared "protected".  I noticed that
 comment is missing here, and thought I'd mention it in case you consider
 it important enough to add.
 >
 Addressed in r2554.  Also for session.h.

 > Minor nit 2:  Seeing that the methods in !XfroutClient had been changed
 from non-virtual to virtual confused me for a bit; I thought there must be
 another derived class somewhere that extended !XfroutClient them, and was
 wondering if I was missing part of the code.  I'd appreciate a comment
 explaining why they're virtual.
 >
 Hmm, I remember a discussion on jabber about the (redundant) virtual
 declaration in a derived class.  I think this is a project-wide style
 guideline issue, rather than a nit to be discussed and addressed case by
 case basis.  I'll bring this topic to bind10-dev.  At the moment I'll keep
 the code unchanged.

 > Index: src/bin/auth/auth_srv.cc
 >
 > "if (is_axfr_connection_established_)" is incorrect English ;)
 >
 > More seriously, that flag only has to do with xfrout, not xfrin, so
 maybe it should have a more specific name?  "xfrout_connected" or
 something.
 >
 It was named so in the original 221 branch (not by me), and I don't have a
 preference.  I tend to agree xfrout_connected sounds better, so I changed
 it.  r2555.

 > I'm not clear on why xfrout_client needs to be declared by the caller
 and passed into the !AuthSrv constructor, as opposed to having it
 instantiated by the !AuthSrvImpl constructor and leaving the !AuthSrv
 constructor's signature unchanged.  I don't see any place where
 xfrout_client is accessed from outside !AuthSrv.
 >
 It's for testing.  I've commented on this in auth_srv.h.  r2555.

 > Index: src/bin/auth/asio_link.h
 >
 > +namespace asio {
 > +class io_service;
 > +namespace ip {
 > +class address;
 > +}
 > +}
 >
 > Does the above code serve a useful purpose?  I commented it out and
 found it still compiled fine, though maybe other compilers would have
 problems.  If it's needed, please add a comment explaining the reason.
 >
 Ah, good catch.  We don't need the forward declation for ip::address any
 more.  Removed.  We still need io_service (this style of forward
 declartion is used in other places, but added a comment anyway).  If a .cc
 compiles without this, it's probably because some other header file
 already declares it.

 In a longer term, however, we should (be able to) obsolete
 IOService::get_io_service(), at which point we can remove the declaration
 completely.

 Addressed in r2556 (and r2557).

 > There were some minor grammatical errors in the comment explaining the
 asio_link class (missing paragraph breaks, singular/plural
 disagreement)--I can just fix them in the branch.
 >
 Please feel free to make such level of changes.  I think it's okay to make
 the changes directly on trunk (assuming this branch is merged to trunk
 soon).

 I've quickly reread asio_link.h, and made a copule of minor editorial
 fixes myself (r2561), but there are probably more.

 > The comments about IOEndpoint say that it's a wrapper for both
 ip::address and for the tcp/udp endpoint classes.  I think the first one
 is a mistake and will remove it in the branch.
 >
 Ah, right.  I guess it was a leftover from an intermediate version of the
 branch.  Thanks for the fix.

 > Comment on IOEndpoint::getAddress() says the return value is "not a real
 object" but appears to mean that it is one.  I'm fixing the comment.
 >
 Good catch, that's correct.

 > The static IOSocket::getDummy* methods seem rather unusual; why can't
 there just be a derived class !DummySocket?  I'm not seeing why this needs
 to be in the base class API.
 >
 This is because I generally wanted to make this class opaque: the
 applications won't instantiate these (derived) classes, and they'll
 normally use instantiated sockets within asio_link (based on the real asio
 object).  Dummy objects are exceptions for testing purposes.

 That said, the decision on this point is in flux.  When we generalize the
 wrapper interface we may want to allow the apps to instantiate specific
 derived socket classes directly.  At that point we might revisit the
 policy.

 At the moment I noted this point as comments. r2558.

 > +    /// This method blocks until the \c stop() method is called via
 some
 > +    /// handler.
 >
 > It does some other stuff too, right? :)
 >
 Right, and I saw you updated it.  Thanks.

 > Index: src/bin/auth/asio_link.cc
 >
 > This code seems fine...
 >
 > +        asio_endpoint_placeholder_(
 > +            new
 tcp::endpoint(ip::address::from_string(address.toText()),
 > +                              port)),
 > +        asio_endpoint_(*asio_endpoint_placeholder_)
 >
 > ...but I don't understand the point.  Why not have asio_endpoint_ be a
 real object instead of a reference, and construct it in the normal way,
 without using "new" and "delete"?  (See attached diff.)
 >
 We could do it (and admittedly it's simpler), but I didn't chose that
 approach due to the copy overhead.  For TCP endpoints it might not be a
 big deal, but we'd probably want to avoid the overhead for every UDP
 query.

 Admittedly, though, this might be a premature optimization.  If you think
 we should keep the initial implementation simpler and revisit it if
 necessary with benchmark results, I'm fine with that.

 At the moment, I've added some comments on this point. r2559.

 > Index: src/bin/xfrin/xfrin.py.in
 >
 > +            # The default RR class is IN.  We should fix this so that
 > +            # the class is passed in the command arg (where we specify
 > +            # the default)
 >
 > This comment appears to be outdated; it seems as if the rrclass ''is''
 passed in the command arguments now.
 >
 This is only partially true: rrclass is passed only in the notify case.
 I've updated the comment to indicate it's not *always* the case.

 > Index: src/bin/auth/tests/asio_link_unittest.cc
 >
 > Two of the tests currently fail:
 >
 > [  FAILED  ] IOServiceTest.badPort
 > [  FAILED  ] IOServiceTest.unavailableAddress
 >
 Hmm, I'm not sure why badPort failed.  I guess your environment is Linux,
 but it passed on bind10.isc.org.  I'll keep this test case for now.  If
 any of the regular builders reports a failure, I'll look into it.  If it's
 not identified and fixed by the time you're back, please report it as a
 separate new ticket.

 I found why it failed for unavailableAddress, and fixed it. (r2553).

-- 
Ticket URL: <http://bind10.isc.org/ticket/221#comment:26>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development