BIND 10 #2470: use MasterLoader in memory::ZoneDataLoader

[BIND 10 Development]

#2470: use MasterLoader in memory::ZoneDataLoader
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Regarding the changelog, maybe it's worth to have it. But I don't know if
 user-facing channel like changelog should contain names of classes in the
 code.

 The most significant problem with the branch is, the
 master_loader_callbacks.cc
 isn't in the repository. Could you, please, commit it?

 Also, I have several mostly minor comments on the code.

 The includes in `zone_data_loader.cc` contain `rrcollator.h` twice:
 {{{#!diff
 +#include <dns/master_loader.h>
 +#include <dns/rrcollator.h>
  #include <dns/rdataclass.h>
  #include <dns/rrset.h>
 -#include <dns/masterload.h>
 +#include <dns/rrcollator.h>
 }}}

 Also, the `rrcollator.cc` should include the `rrcollator.h` as the first
 header, not name.h, to ensure the `rrcollator.h` compiles without other
 dependencies.
 {{{#!c++
 #include <dns/name.h>
 #include <dns/rdataclass.h>
 #include <dns/rrcollator.h>
 #include <dns/rrclass.h>
 #include <dns/rrtype.h>
 #include <dns/rrttl.h>
 #include <dns/rdata.h>
 #include <dns/rrset.h>
 }}}

 Should this produce a warning of some kind?
 {{{#!c++
     } else if (current_rrset_->getTTL() != rrttl) {
         // RRs with different TTLs are given.  Smaller TTL should win.
         current_rrset_->setTTL(std::min(current_rrset_->getTTL(), rrttl));
     }
 }}}

 Looking at the RRCollator class makes me wonder how much performance we
 lose in the huge amount of pimpls we have. Is it really needed here? And
 in case it is needed, the RRCollator class should probably be made
 uncopyable, because this way we may get some nasty memory management bugs
 when someone copies it accidentally.

 The brief here is missing text:
 {{{#!c++
     /// \brief
     typedef boost::function<void(const RRsetPtr& rrset)> AddRRsetCallback;
 }}}

 I don't see anything wrong about the name `finish`, but I think a more
 intuitive name could be `flush`.

 And it seems you thought of it too, according to this comment:
 {{{#!c++
     /// Like \c flush(), this \c AddRRCallback functor propagates any
 exception
     /// thrown from the callback.
 }}}

 Is there a reason why these are not initialized in the initializer list?
 {{{#!c++
     {
         a_rdata1_ = createRdata(RRType::A(), rrclass_, "192.0.2.1");
         a_rdata2_ = createRdata(RRType::A(), rrclass_, "192.0.2.2");

         txt_rdata_ = createRdata(RRType::TXT(), rrclass_, "test");

         sig_rdata1_ = createRdata(RRType::RRSIG(), rrclass_,
                                   "A 5 3 3600 20000101000000
 20000201000000 "
                                   "12345 example.com. FAKE\n");
         sig_rdata2_ = createRdata(RRType::RRSIG(), rrclass_,
                                   "NS 5 3 3600 20000101000000
 20000201000000 "
                                   "12345 example.com. FAKE\n");
     }
 }}}

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