BIND 10 #2436: update datasrc::ZoneLoader so it performs post load checks using validateZone()

[BIND 10 Development]

#2436: update datasrc::ZoneLoader so it performs post load checks using
validateZone()
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130122
         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

 Replying to [comment:7 jinmei]:
 > '''dependency note'''
 >
 > In my review on #2435, I raised a discussion about whether it make
 > more sense to have the updater create a collection, rather than
 > constructing a collection from an updater.  Depending on the result of
 > the discussion, this branch may also have to be updated (although the
 > effect should be pretty local and small).

 Noted. I'd solve it at merge time, since it wouldn't compile and it is a
 trivial change.

 > '''b10-loadzone'''
 >
 > We also need to update the man page.  It talks about the limitation on
 > validation (post load checks) of the initial version.  It's now fixed.
 > Also check the guide (I don't remember, but there may be the same
 > issue).

 I updated the man page (simply removed the bug entry), but I didn't find
 anything in the guide.

 > - The `Name` can be a reference (avoiding copy) here:
 > {{{#!cpp
 >         const dns::Name zone_name(updater_->getFinder().getOrigin());
 > }}}
 >   (although the saved copy overhead may be marginal in this context).

 Can it be really? The `getOrigin()` returns an object, not reference. As
 it is
 a return value, it is temporary, so it'd be taking reference of a
 temporary. I
 know it is not allowed taking pointer of such value, so I thought it is
 the
 same with reference. Or is there some hidden trick in C++?

 > - what's the purpose of this?
 > {{{#!cpp
 >             // Validation failed.
 >             loaded_ok_ = false;
 > }}}
 >   loaded_ok_ doesn't seem to be used after this point anyway.

 You think about such states in transition diagrams, I do it by assigning
 logical meanings to variables. And, as the name suggests, holds if there
 was an
 error or not during the load. When following this logic, I set there was
 an
 error and I don't have to examine if it is ever used later or not, because
 I'm
 just being consistent with the purpose of the variable. It is just simpler
 for
 me this way.

 > '''zone_loader_unittest'''
 >
 > - This doesn't have to be `BasicRRset`.  A mere `RRset` suffices:
 > {{{#!cpp
 >         RRsetPtr newRRset(new isc::dns::BasicRRset(rrset.getName(),
 > }}}

 Actually, I don't think „mere“ is in place here, since RRset inherits from
 `BasicRRset`. I used `BasicRRset` as it is the most primitive class that
 suffices. I don't really need the signatures offered by full RRset.

 > - in copyValidation, shouldn't even prepareSource() fail once we
 >   complete #2499?

 Yes, it would. That's what the TODO says, then some new way would have to
 be
 used to load the data.

 > - shouldn't we check the previous version of zone is intact when check
 >   zone fails?

 It is checked:
 {{{#!c++
 EXPECT_FALSE(destination_client_.commit_called_);
 }}}

 > - not for this branch, but I happened to notice:
 > {{{#!cpp
 >         // Currently, load uses an urelated implementation. In the long
 term,
 > }}}
 >   "urelated" seems to be a typo.  unrelated? (but I'm not sure what
 >   "unrelated implementation" means).

 It means the in-memory uses something else to load the RRs than what we
 test here.

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