BIND 10 #2218: update InMemoryZoneFinder::findNSEC3 using memory-efficient version

[BIND 10 Development]

#2218: update InMemoryZoneFinder::findNSEC3 using memory-efficient version
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120925
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:10 muks]:

 > > - The original code takes care of the case where there's no NSEC3 (but
 > >   there maybe NSEC3PARAM).  Shouldn't we do the same?
 >
 > We do in the loader. I am not following where in the original code that
 you are referring to.

 I meant this by "original":
 {{{#!cpp
     const NSEC3Map& map = impl_->zone_data_->nsec3_data_->map_;
     if (map.empty()) {
         isc_throw(DataSourceError,
                   "findNSEC3 attempt but zone has no NSEC3 RR: " <<
                   impl_->origin_ << "/" << impl_->zone_class_);
     }
 }}}
 and, as far as I can see, loader doesn't ensure we can skip the same
 check.  See relevant comments for the tests below.

 Rest of my code comments follow::

 '''domaintree.h'''

 - I don't see the need for `getSubTreeRoot()`.  Why was this added?
   (I've not reviewed corresponding tests as I was not sure if we
   really need this method).
 - Why did you change the type of node_count_ to size_t?  I suspect
   sizeof(size_t) can often be 8 for 64-bit machines, and with the
   existence of `needsReturnEmptyNode_` it would make the
   `sizeof(DomainTree)` unnecessarily large.  It can be a waste if we
   support millions of zones.  I admit 'unsigned int' might not be a
   good choice either, though.  Maybe the best one is uint32_t, and
   explain why we specifically choose it.

 '''memory_client.h/cc'''

 - maybe not really related to this branch, but I suspect we shouldn't
   make findZone2() a const member function, because it effectively
   returns a mutable internal state.  Also, it would better to be not
   virtual (so it won't be misleading).  I'd probably choose a
   different name.  And, if it's mainly for tests I'd note that in the
   documentation (if it's expected to be used for general use, I think
   we should reconsider it - returning internal zone data seems to be a
   risky interface).
 - This now-removed comment makes me nervous
 {{{#!cpp
 -    // This variant of findZone() is not implemented and should be
 -    // removed eventually. It currently throws an exception. It is
 -    // required right now to derive from DataSourceClient.
 -    virtual isc::datasrc::DataSourceClient::FindResult
 -    findZone(const isc::dns::Name& name) const;
 }}}
   It said "should be removed eventually", but this branch did the exact
   opposite.  What happened?

 '''treenode_rrset'''

 - I don't see why we need to implement getRRsig() in this branch.  But
   if you wanted to implement it, please be responsible for providing
   tests.  The documentation needed to be updated too. (But I did both
   in #2219, so you don't have to address these in this branch).

 '''zone_finder_unittest.cc'''

 - I'd avoid initializing nsec3_hash_map in the namespace level.  I've
   committed my suggested change to address this (2fdacd8).
 - should this be removed?
 {{{#!cpp
     // This needs to be updated and checked when implementing #2118
 }}}

 - this is invalid if salt_data_2 is empty:
 {{{#!cpp
                  (std::memcmp(&salt_data_2[0], salt_data, salt_len) != 0))
 {
 }}}

 - I don't understand this comment:
 {{{#!cpp
         // We assume that rrsig has already been checked to match rrset
         // by the caller.
 }}}

 - I don't think this test really does what it claims:
 {{{#!cpp
     // Only having NSEC3PARAM isn't enough
     addZoneData(textToRRset("example.org. 300 IN NSEC3PARAM "
                             "1 0 12 aabbccdd"));
 }}}
   because in the actual implementation, "only having NSEC3PARAM"
   enables `NSEC3Data`.  So findNSEC3 actually wouldn't throw.

 - the comment doesn't really seem to be 100% accurate.  It's indeed
   for `InMemoryZoneFinder`, but in this context it's NSEC3 specific.
 {{{#!cpp
 /// \brief Test fixture for the InMemoryZoneFinder class
 class InMemoryZoneFinderNSEC3Test : public InMemoryZoneFinderTest {
 }}}

 - can you add some comments on findNSEC3Walk and its the data?  What's
   the point of the test, meaning of `TestData` member variables, how
   the hash values are chosen (whether they are randomly chosen or
   intended to test some specific case - I suspect the latter, but it's
   not immediately obvious from the definitions).

 '''nsec3hash'''

 Frankly, I don't like this hack.  It breaks some design principles of
 the class (e.g., reusing internal resources for multiple hash
 calculations - for that matter, this branch doesn't update the
 documentation on this).  Also, defining `NSEC3Hash::calculate` for
 SHA1 only seems ad hoc.

 To me, a better approach is to extend the factory interface so that we
 can create an `NSEC3Hash` instance from the algorithm, iterations and
 salt.

 the in-memory implementation would have a function pointer to a
 specific factory instance for the normal and test cases, not the
 calculation function itself.

 '''zone_finder.h'''

 nsec3_calculate_: In general, I don't like to introduce a public or
 protected member variable, especially if it's mutable, as it can
 easily break class integrity due to a careless application or derived
 class implementation.  In this case, if there's no better way to allow
 tests to use a faked calculator, please at least leave comment that
 this protected member variable is only intended for test purposes
 (isn't it?), and other derived class shouldn't try to tweak it.  Also,
 I suspect the in-memory finder class isn't really expected to be
 further derived except for testing.  If that's the case, please also
 document that.

 '''zone_finder.cc'''

 In addition to other points that were already discussed, I've noticed
 the same patter like this repeated:
 {{{#!cpp
             const RdataSet* rdataset = node->getData();
             assert(rdataset != NULL);
             assert(rdataset->type == RRType::NSEC3());

             ConstRRsetPtr closest = createTreeNodeRRset(node, rdataset,
                                                         getClass());
 }}}
 and it makes the method longer and less readable.  I'd introduce a
 helper function to unify the logic:
 {{{#!cpp
 ConstRRsetPtr
 createNSEC3RRset(const ZoneNode* node, const RRClass& rrclass) {
     const RdataSet* rdataset = node->getData();
     // add comment why we can assert these
     assert(rdataset != NULL);
     assert(rdataset->type == RRType::NSEC3());

     return (createTreeNodeRRset(node, rdataset, rrclass));
 }
 }}}
 and replace the above with this:
 {{{#!cpp
             ConstRRsetPtr closest = createNSEC3RRset(node, getClass());
 }}}

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