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
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:11 vorner]:

 > > Yeah, I've realized we need it while working on #2429.  I implemented
 > > it, but there's one controversial point: I used
 > > `MasterLoaderCallbacks`, but the collator class cannot provide the
 > > source name or the line number because these parameters are lost when
 > > the RR callback is called.  If we want to include the correct values
 > > for these in the warning, we'll need some kind of extension in
 > > `MasterLoader`.
 >
 > Will you add the extension here, or in some other ticket? The <unkown
 source>
 > looks really awkward.

 I can think of a couple of possibilities, but I'm afraid these are not
 a trivial task.  Among them I guess the least ugly approach is to have
 `MasterLoader` detect it and issue the corresponding callback.  This
 will require `MasterLoader` to maintain another set of states, which
 reduces the main advantage of separating the collator, but any other
 approach requires some way to pass the information of the source name
 and line from `MasterLoader` to its caller, which will make the API
 dirtier and the implementation more complicated.

 Meanwhile, #2429 resulted in maintaining some kind of state of what's
 the most recently used TTL, and I suspect #2427 will introduce a
 similar state for the owner name.  So it won't be that trickier to
 detect the TTL change for RRs of the same RRset in `MasterLoader` once
 we complete both tasks.

 Doing this job in `MasterLoader` has another advantage that we can do
 this check for the database-based data source loader in the unified
 implementation.

 To this end, my suggestion is to remove the additional callback from
 the collator, and create a ticket to this extension.

 > > First off, I don't think performance is not a big deal in this case
 > > for the expected usage.  Regarding whether we need it, it's hard to
 > > tell, and to some extent a matter of preference.  [...]
 >
 > I don't disagree with your points, but it has the other side too.

 I know, so it's a YMMV thing.  In this particular case I'd not
 necessarily insist pimpl is absolutely better, but I don't see much
 problem in it either.  So I'll just keep it as it is.

 > And another thing, is the original masterload function unused now?
 Should we
 > remove it?

 At least not yet, because it's used in many parts of tests.  There's a
 cleanup ticket related to this topic: #2381.

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