BIND 10 #1576: implement ZoneFinder::findNSEC3 in in-memory data source

[BIND 10 Development]

#1576: implement ZoneFinder::findNSEC3 in in-memory data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120221
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  6
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:12 jinmei]:
 > > I'm not sure I like the NSAC3HashCreator approach. If it is for
 testing only, it looks like an overkill, introducing yet another abstract
 base class, which is 2 already for the simple thing of calculating a hash.
 If it was to provide some flexibility, it would be OK, but it doesn't. If
 you replace the creator for some experimental hash algorithm, you lose the
 old one. Not to say that this wouldn't work if two people provided two
 plugins for two different algorithms.
 >
 > I see points of the overkill argument.  I also didn't like relying on
 > the essentially singleton stuff.  But the alternative would be to
 > add a some kind of hook in `InMemoryZoneFinder` so the user of it (in
 > this case the unit tests) can specify a possibly specialized
 > `NSEC3Hash` object, just like we pass `AbstractSession` to
 > `ModuleCCSession` even though the only reason for using the base class
 > is convenience for tests.  Since the hash calculation logic is
 > basically static in practice, however, I didn't like adding a
 > tweakable knob for that purpose either (my assumption is that if
 > someone wants to play with experimental hash calculator, possibly for
 > using a new/experimental hash algorithm, they'll replace the creator
 > at the beginning of their application and use it all the time).  Hence
 > the current approach.

 I don't know. If I wanted to play with a hash algorithm, I'd want the rest
 to keep working and check how it works in practice (eg. plugging it into
 b10-auth or b10-resolver and putting it online somewhere). So the approach
 of replacing it doesn't seem to work.

 I understand it is too early to worry about it, but then, I think the
 comment should claim testing purposes only for now, because I don't see
 how this would help in the case of the experiments. It might be worth
 noting the interface might change for allowing of easy plugging in of new
 algorithms.

 > In the end, if you don't like the current creator design, my next
 > proposal is to add a new `InMemoryZoneFinder::setNSEC3Hash()` method,
 > which tells the finder to use the given hash object regardless of
 > other parameters of NSEC3 related RRs (noting it's only for testing
 > purposes).  It's ugly, but we can then eliminate the reliance on the
 > singleton creator.

 That doesn't look any better, I'd say it is even worse. I think it is OK
 to keep it this way, but I think it needs to suggests more it is not for
 common use.

 > > Is there any reason why the destructor of InMemoryZoneFinderTest is
 virtual?
 >
 > I thought the destructor for its base class (in gtest lib) was also
 > virtual, and followed the convention we use in other cases.  But we
 > probably don't do this for tests too often, so I'm okay with removing
 > it if it looks too awkward.  In any case, the behavior wouldn't change
 > with or without it.

 Well, it is surprising, because nothing inherits from it (or, it does,
 hidden in the macros, but they would be static objects on the stack, so
 the dynamic resolution would not happen anyway). The first think I did
 when I saw it was looking if the test inherits from something unusual or
 if something inherits from the test. Then I asked if there's some other
 reason I overlooked.

 Anyway, it compiles now, but there's a failing test:
 {{{
 [ RUN      ] InMemoryZoneFinderTest.loadAndFindNSEC3
 memory_datasrc_unittest.cc:1975: Failure
 Value of: result1.closest_labels
   Actual:
 Expected: 3
 }}}

 I was surprised by the empty „Actual“ field and put it into a debugger. It
 shows that the closest_labels is a char (so it would be nice to turn it to
 integer, or, why is the variable char-sized anyway? I know it can be up to
 255 anyway, but is there a need to save space, if the return value is just
 an ephemeral object?). And the debugger showed it was 4 at the time.

 Thank you

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