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

 Replying to [comment:9 jinmei]:
 > Hmm, the motivation looks reasonable, so I did it, but I have some
 > comments.
 > - in the general case it wouldn't be clear which ordering makes most
 >   sense for this purpose.  So there may be some implicit assumption
 >   that we do this in "some_module.cc" for "some_module.h", but it's
 >   not clear from the above comment.
 > - likewise, the intent wouldn't be so obvious if we just reorder the
 >   header files.  Maybe some people notice the correlation of the
 >   prefix ("some_module"), but that's not super clear, and the
 >   background intent would be even less clearer.  So, if we do this
 >   practice I think we need to leave a comment to clarify the intent (i
 >   did it in this branch); otherwise someone may reorder them again as
 >   a cleanup, e.g., in the alphabetical order.
 > - also related, if we do this, it should be consistently adopted
 >   project-wide, and should be documented in the style guideline.

 Actually, my impression of the decision about order was that it included
 even
 the fact that the own header (something.h inside something.cc) should be
 done
 first and that was one of the biggest motivations for the decision. I do
 follow
 that and it seems most files are written that way. But looking at the
 style
 guideline, it's not written there.

 So I'll send an email about it.

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

 > 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 realize I'm at the
 > side of preferring pimpl in general in the project, because it helps
 > reduce dependency a lot; in particular, it helps avoid leaking
 > dependencies on std, boost or other external libraries via the public
 > header file.  It then often helps reduce compile time (which can be a
 > big plus for such a large package like BIND 10), and helps reduce the
 > risk of seeing strange runtime behavior such as binary
 > incompatibilities (you might remember we have had such weird things by
 > leaking ASIO definitions through our header files).

 I don't disagree with your points, but it has the other side too. The
 pimpl is
 more code to type (because you need to do the indirect methods), makes the
 code
 slower, fragments memory and it is less readable, since one needs to look
 through yet another layer of objects. So I don't oppose usage of pimpl,
 it's
 just I don't like too much of it everywhere. This class looks rather
 simple and
 the dependencies can be solved by a forward declaration of AbstractRRset.
 But
 I'll leave it up to you.

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