BIND 10 #84: review: auth server

[BIND 10 Development]

#84: review: auth server
--------------------------+-------------------------------------------------
 Reporter:  jinmei        |        Owner:  each                         
     Type:  task          |       Status:  closed                       
 Priority:  major         |    Milestone:  03. Authoritative-only server
Component:  Unclassified  |   Resolution:  fixed                        
 Keywords:                |    Sensitive:  0                            
--------------------------+-------------------------------------------------
Changes (by jinmei):

  * status:  assigned => closed
  * resolution:  => fixed


Comment:

 > I have now read src/bin/auth, and approve it for Y1.

 Okay, thanks.  I'm going to close this ticket, but answer the comments for
 the record.

 > A couple of notes for Y2:
 > - Needs more comments

 Absolutely:-) And, actually, I think we should revisit the whole design
 and implementation of the main auth server code in Y2.  The DNSPORT point
 would also be covered in that effort.

 > - DNSPORT, it seems to me, should be an aspect of AuthSrv rather than a
 static variable in main.cc

 > - I don't object, but I don't really understand the benefit of pimpl in
 AuthSrv

 IMO the question should be reversed: In general we should ask why if a
 class doesn't use pimpl, and suggest using it unless there's specific
 reason for not doing so.

 The strongest reason for that is because with pimpl we can hide depedency
 details from public header files.  A class definition that doesn't use
 pimpl tends to contain object member variables defined outside the module
 or package.  In the case of AuthSrv class, if it were not using pimpl, it
 would contain

     std::string db_file_;
     MetaDataSrc data_sources_;
     ConstDataSrcPtr cur_datasrc_;
 (in addition to other, less harmful members)

 Since these are real objects (i.e. not a pointer or reference) we need to
 include the corresponding header files for the definitions of these
 classes.

 This will increase compile time because every time we modify data_source.h
 (for MetaDataSrc) we'll also need to compile auth_srv.cc again even if the
 public interface isn't changed.  Also, since MetaDataSrc is a pretty big
 class that also possibly depends on other classes, the indirect dependency
 may increase the bulid overhead further.

 Standard library objects such as std::string should be more stable in this
 sense, but they cause other kinds of problem: portability.  Without pimpl,
 a binary image of an AuthSrv object contains a binary image of std::string
 object at the build time (of BIND10), which may not be compatible with the
 binary image of std::string when it's linked with a third-party program.

 ConstDataSrcPtr is actually a typedef of boost::shared_ptr, so the
 probability problem could be even worse than std::string as it's a moving
 target.

 Of course, there are cons with pimpl.  One big issue with it is that
 construction, copy and assignment will be more complicated and less
 efficient because these will require dynamic memory allocation in addition
 to initialization of the member variables.  So, for a class that is
 frequently copied *and* used in performance sensitive path, we should be
 careful about using the pimpl approach.  This is one reason why I didn't
 adopt pimpl for the dns::Name class.

 The AuthSrv class doesn't have this issue.  Actually, it's essentially a
 singleton object, much less being copied or assigned to other variables,
 or constructed/destructed multiple times.

 Considering these points and other, relatively minor complexity issues due
 to pimpl, I believe this is a case where "we should use pimpl unless
 there's a reason for not doing so".

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