Opened 7 years ago

Closed 7 years ago

#2218 closed task (fixed)

update InMemoryZoneFinder::findNSEC3 using memory-efficient version

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20121023
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: scalable inmemory
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 10.75 Internal?: no

Description

(I've noticed this task should have been included for the feature
but has been missing).

After #2107 the NSEC3 name space will also be maintained in a
DomainTree, not std::map (from std::string to rrset). Likewie
the zone's NSEC3 parameter won't be maintained in the form of
NSEC3Hash object. So we need to update the findNSEC3() method
accordingly. Doing something similar to getClosestNSEC(), etc.

Subtickets

Attachments (2)

dump.png (23.5 KB) - added by muks 7 years ago.
The NSEC3 ZoneTree? that is built by our tests (inside NSEC3Data)
memory_client.diff (5.6 KB) - added by jinmei 7 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 7 years ago by jinmei

  • Type changed from defect to task

comment:2 Changed 7 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:3 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120904

comment:4 Changed 7 years ago by shane

  • Owner set to muks
  • Status changed from new to assigned

comment:5 Changed 7 years ago by muks

Up for review. It is in the trac2218_2 branch.

When you are following the code, note that the NSEC3 tree is again a tree-of-trees, and we work only in the sub-tree under the origin node. NSEC3 names have 1+ the number of origin labels, and all of them are in the sub-tree under the origin node. Some changes to DomainTree were introduced to help with this.
Insertion into the NSEC3 tree has to be done with the full and absolute names, as these are later used by TreeNodeRRset::getName() as the RdataSet itself contains no name.

Changed 7 years ago by muks

The NSEC3 ZoneTree? that is built by our tests (inside NSEC3Data)

comment:6 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

comment:7 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:8 Changed 7 years ago by jinmei

I've not completed full review, but found several substantial issues.
So I'm dumping the current review comments and giving ticket back to
mukund. Please address (or discuss to resolve) these points first.

general

  • According to the commit sequence, this branch doesn't seem to be developed "test driven". It's not what we should do.
  • zone_finder_unittest and faked_nsec3 test have a lot of duplicates. Please unify them.

memory_client.cc

  • In addNSEC3(), I believe we can (now) simply add the owner name of the NSEC3 RRset:
            ZoneNode* node;
            nsec3_data->insertName(mem_sgmt_, rrset->getName(), &node);
    
    In fact, all tests pass with this version (and if this isn't supposed to work, it means we don't have sufficient tests).
  • I don't like to have something like iterateSHA1 in the zone_finder code. The data source code is basically supposed to use a higher level abstraction, the NSEC3Hash class. The current code doesn't only rely on the low level concept, but also rely on in-house SHA1 implementation, which we'd probably replace with Botan.
  • The original code takes care of the case where there's no NSEC3 (but there maybe NSEC3PARAM). Shouldn't we do the same?
  • maybe a matter of taste, but this expression doesn't look very readable (very long and with ?:):
            const std::string hlabel = (nsec3_calculate_)(
                ((labels == qlabels ?
                  name : name.split(qlabels - labels, labels)),
                 nsec3_data->iterations,
                 nsec3_data->getSaltData(),
                 nsec3_data->getSaltLen());
    
    I'd introduce a variable to the main expression concise:
            const Name& hname = (labels == qlabels ?
                                 name : name.split(qlabels - labels, labels));
            const std::string hlabel = (nsec3_calculate_)(
                hname, nsec3_data->iterations, nsec3_data->getSaltData(),
                nsec3_data->getSaltLen());
    
  • also maybe a matter of taste, but maybe it's slightly more lightweight if define chain outside the for loop and clear it:
        ZoneChain chain;
        for (unsigned int labels = qlabels; labels >= olabels; --labels) {
    ...
            //ZoneChain chain; <= instead of this
            chain.clear();
    
  • I'd move the definition of tree outside of the for loop:
        const ZoneTree& tree = nsec3_data->getNSEC3Tree();
        for (unsigned int labels = qlabels; labels >= olabels; --labels) {
    ...
            //const ZoneTree& tree = nsec3_data->getNSEC3Tree();
    
  • this seems to be unnecessarily expensive:
            const ZoneTree::Result result =
                tree.find(Name(hlabel + "." + getOrigin().toText()), &node, chain);
    
    due to the mix of toText(), string concatenation and name construction from string. I'd do it:
            const ZoneTree::Result result =
                tree.find(Name(hlabel).concatenate(getOrigin()), &node, chain);
    
  • I'd rename set to rdataset or rdset. set sounds too generic.
  • couldn't the construction of next be simpler, like this?
                ConstRRsetPtr next = (covering_node == NULL) ? ConstRRsetPtr() :
                    createTreeNodeRRset(covering_node, covering_node->getData(),
                                        getClass());
    
  • this implementation assumes ZoneNode->getData() is non NULL and is the NSEC3 RRset. That should be correct, but could be fragile as it heavily relies on how the NSEC3 is constructed. I'd be a bit more proactive, e.g. by asserting the type is NSEC3.
  • these comments don't match the actual code any more. please update:
                // If the given hash is larger than the largest stored hash or
                // the first label doesn't match the target, identify the
                // "previous" hash value and remember it as the candidate next
                // closer proof.
    ...
                    // Otherwise, H(found_entry-1) < given_hash < H(found_entry).
                    // The covering proof is the first one (and it's valid
                    // because found is neither begin nor end)
    
    This type of mismatch seems to be commonly observed in your branches. If this is something you often forget to check, please include in your personal check list for future tasks if that is mainly updating existing code.
  • the non-exact match case seems to have many issues.
  • first off, it doesn't seem correct. Consider a name whose hash is 02UDEMVP1J2F7EG6JEBPS17VP3N8I58H. If we do non recursive search for this name, the covering hash should be 01UDEMVP1J2F7EG6JEBPS17VP3N8I58H, but this implementation actually returns some bogus (not just incorrect) result.
  • secondly, I don't like to expose too much of DomainTreeNode internals. Specifically, I don't like to make predecessor() and successor() just for this purpose. Likewise, I don't like to introduce things like getLargestInSubTree() as a public method of DomainTreeNode. In general, I prefer extending the node chain interfaces for operations that require moving inside the tree. Also, due to the special structure of the NSEC3 tree, "in sub tree" property is not really necessary. While exploiting this fact itself may be controversial, I personally like to exploit it than tweaking the node class (and this code already exploits some of the special nature of the NSEC3 anyway).
  • Even if this code were accurate, it seems to be unnecessarily expensive. It always compute predecessor() and successor(), which are not super cheap. In the vast majority cases we'd have a non NULL previous node, and we should simply be able to use it in this case.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:10 follow-ups: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

I've not completed full review, but found several substantial issues.
So I'm dumping the current review comments and giving ticket back to
mukund. Please address (or discuss to resolve) these points first.

general

  • According to the commit sequence, this branch doesn't seem to be developed "test driven". It's not what we should do.

Sorry about that. In many cases I was working on the problem at hand (which used the method being added), and had to come back to properly add testcases.

  • zone_finder_unittest and faked_nsec3 test have a lot of duplicates. Please unify them.

Done.

memory_client.cc

  • In addNSEC3(), I believe we can (now) simply add the owner name of the NSEC3 RRset:
            ZoneNode* node;
            nsec3_data->insertName(mem_sgmt_, rrset->getName(), &node);
    
    In fact, all tests pass with this version (and if this isn't supposed to work, it means we don't have sufficient tests).

You are right. As we use ZoneTree::find() now, there's no need to change the first label to uppercase. Many of the review issues are because this was a direct translation of the map based findNSEC3(), but I've updated it now.

  • I don't like to have something like iterateSHA1 in the zone_finder code. The data source code is basically supposed to use a higher level abstraction, the NSEC3Hash class. The current code doesn't only rely on the low level concept, but also rely on in-house SHA1 implementation, which we'd probably replace with Botan.

These have been moved to the NSEC3Hash class. I can't think of a better way to do it than have a static function there as we deal with raw data.

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

  • maybe a matter of taste, but this expression doesn't look very readable (very long and with ?:):
            const std::string hlabel = (nsec3_calculate_)(
                ((labels == qlabels ?
                  name : name.split(qlabels - labels, labels)),
                 nsec3_data->iterations,
                 nsec3_data->getSaltData(),
                 nsec3_data->getSaltLen());
    
    I'd introduce a variable to the main expression concise:
            const Name& hname = (labels == qlabels ?
                                 name : name.split(qlabels - labels, labels));
            const std::string hlabel = (nsec3_calculate_)(
                hname, nsec3_data->iterations, nsec3_data->getSaltData(),
                nsec3_data->getSaltLen());
    

Updated.

  • also maybe a matter of taste, but maybe it's slightly more lightweight if define chain outside the for loop and clear it:
        ZoneChain chain;
        for (unsigned int labels = qlabels; labels >= olabels; --labels) {
    ...
            //ZoneChain chain; <= instead of this
            chain.clear();
    

Updated.

  • I'd move the definition of tree outside of the for loop:
        const ZoneTree& tree = nsec3_data->getNSEC3Tree();
        for (unsigned int labels = qlabels; labels >= olabels; --labels) {
    ...
            //const ZoneTree& tree = nsec3_data->getNSEC3Tree();
    

Updated.

  • this seems to be unnecessarily expensive:
            const ZoneTree::Result result =
                tree.find(Name(hlabel + "." + getOrigin().toText()), &node, chain);
    
    due to the mix of toText(), string concatenation and name construction from string. I'd do it:
            const ZoneTree::Result result =
                tree.find(Name(hlabel).concatenate(getOrigin()), &node, chain);
    

Updated.

  • I'd rename set to rdataset or rdset. set sounds too generic.

Done.

  • couldn't the construction of next be simpler, like this?
                ConstRRsetPtr next = (covering_node == NULL) ? ConstRRsetPtr() :
                    createTreeNodeRRset(covering_node, covering_node->getData(),
                                        getClass());
    

Updated.

  • this implementation assumes ZoneNode->getData() is non NULL and is the NSEC3 RRset. That should be correct, but could be fragile as it heavily relies on how the NSEC3 is constructed. I'd be a bit more proactive, e.g. by asserting the type is NSEC3.

Added.

  • these comments don't match the actual code any more. please update:
                // If the given hash is larger than the largest stored hash or
                // the first label doesn't match the target, identify the
                // "previous" hash value and remember it as the candidate next
                // closer proof.
    ...
                    // Otherwise, H(found_entry-1) < given_hash < H(found_entry).
                    // The covering proof is the first one (and it's valid
                    // because found is neither begin nor end)
    
    This type of mismatch seems to be commonly observed in your branches. If this is something you often forget to check, please include in your personal check list for future tasks if that is mainly updating existing code.
  • the non-exact match case seems to have many issues.
  • first off, it doesn't seem correct. Consider a name whose hash is 02UDEMVP1J2F7EG6JEBPS17VP3N8I58H. If we do non recursive search for this name, the covering hash should be 01UDEMVP1J2F7EG6JEBPS17VP3N8I58H, but this implementation actually returns some bogus (not just incorrect) result.
  • Even if this code were accurate, it seems to be unnecessarily expensive. It always compute predecessor() and successor(), which are not super cheap. In the vast majority cases we'd have a non NULL previous node, and we should simply be able to use it in this case.

The code was indeed buggy and has been rewritten. A tree walk test has also been added which looks for hashes on both sides of each node, and also existing nodes (exact matches), both recursive and non-recursive. The old comments are also gone.

  • secondly, I don't like to expose too much of DomainTreeNode internals. Specifically, I don't like to make predecessor() and successor() just for this purpose. Likewise, I don't like to introduce things like getLargestInSubTree() as a public method of DomainTreeNode. In general, I prefer extending the node chain interfaces for operations that require moving inside the tree.

For this case, I want to disagree. DomainTree is more or less a BST with walking capability. predecessor() and successor() are basic operations on such a data structure, and if there is the use-case, I don't think we should force to keep these hidden. The added methods are operations on nodes, and are basic operations. I feel adding them in the node chain would not be ideal. I thought of adding a method like getNSEC3CoverNode() to the node chain, but such things seem to be misplaced in the DomainTree.

Also, due to the special structure of the NSEC3 tree, "in sub
tree" property is not really necessary. While exploiting this
fact itself may be controversial, I personally like to exploit it
than tweaking the node class (and this code already exploits some
of the special nature of the NSEC3 anyway).

I don't follow what you mean by exploit here. Do you mean not have the tree-of-tree structure? TreeNodeRRset gets its name directly from the node in the tree-of-trees.

comment:11 in reply to: ↑ 10 Changed 7 years ago by jinmei

Replying to muks:

Some substantial points first:

  • secondly, I don't like to expose too much of DomainTreeNode internals. Specifically, I don't like to make predecessor() and successor() just for this purpose. Likewise, I don't like to introduce things like getLargestInSubTree() as a public method of DomainTreeNode. In general, I prefer extending the node chain interfaces for operations that require moving inside the tree.

For this case, I want to disagree. DomainTree is more or less a BST with walking capability. predecessor() and successor() are basic operations on such a data structure, and if there is the use-case, I don't think we should force to keep these hidden. The added methods are operations on nodes, and are basic operations. I feel adding them in the node chain would not be ideal. I thought of adding a method like getNSEC3CoverNode() to the node chain, but such things seem to be misplaced in the DomainTree.

First, regarding successor(): at least your revised code doesn't use
successor(), so it can (and I would say should) be private again.
Next, as for predecessor(): you almost reimplemented the logic that
is already available DomainTree::previousNode as a result of using
predecessor(). Apart from whether it makes sense to expose
predecessor(), the resulting code is duplicate and IMO much more
unreadable. The whole re-invented logic can be much more simplified
if you use previousNode:

        } else {
            const ZoneNode* last_node = chain.getLastComparedNode();
            while ((covering_node = tree.previousNode(chain)) != NULL &&
                   covering_node->isEmpty()) {
                ;
            }
            if (covering_node == NULL) {
                covering_node = last_node->getLargestInSubTree();
            }

            if (!recursive) {   // in non recursive mode, we are done.
...

If you agree that this one is simpler and more readable, then we also
don't need to make predecessor() public either, and I'd suggest
keeping it private.

Also, due to the special structure of the NSEC3 tree, "in sub
tree" property is not really necessary. While exploiting this
fact itself may be controversial, I personally like to exploit it
than tweaking the node class (and this code already exploits some
of the special nature of the NSEC3 anyway).

I don't follow what you mean by exploit here. Do you mean not have the tree-of-tree structure? TreeNodeRRset gets its name directly from the node in the tree-of-trees.

What I meant (re-reading it now, I admit the original comment wasn't
clear) is that I'd rather define and use something like
TreeNode* DomainTree::largestNode(), which returns the largest node
in the tree of tree. In the case of the NSEC3 tree, this should be
the same as "largest in subtree" (in the subtree of NSEC3 hash names).

I admit the encapsulation of DomainTree isn't really clean, and we
sometimes allow the applications to use the knowledge of its internal
structure. But when possible, I'd like to hide such details as it's a
"tree of tree" as much as possible, and have applications use
DomainTree just like some kind of abstract container. The concept
of "getting the largest entry" can exist for any type of container
that has the notion of ordering, but getLargestInSubTree looks too
much rely on specific internal details of it. It might be less likely
that we'll fundamentally revise the internal details (e.g., like
changing tree to hash), but if we have choices for some purpose that
have comparable costs, I'd like to choose the one that can hide
internal details more.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by jinmei

Replying to 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":

    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
    -    // 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?
        // This needs to be updated and checked when implementing #2118
    
  • this is invalid if salt_data_2 is empty:
                     (std::memcmp(&salt_data_2[0], salt_data, salt_len) != 0)) {
    
  • I don't understand this comment:
            // 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:
        // 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.
    /// \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:

            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:

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:

            ConstRRsetPtr closest = createNSEC3RRset(node, getClass());

comment:13 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:14 Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:
[snip]

First, regarding successor(): at least your revised code doesn't use
successor(), so it can (and I would say should) be private again.
Next, as for predecessor(): you almost reimplemented the logic that
is already available DomainTree::previousNode as a result of using
predecessor(). Apart from whether it makes sense to expose
predecessor(), the resulting code is duplicate and IMO much more
unreadable. The whole re-invented logic can be much more simplified
if you use previousNode:

        } else {
            const ZoneNode* last_node = chain.getLastComparedNode();
            while ((covering_node = tree.previousNode(chain)) != NULL &&
                   covering_node->isEmpty()) {
                ;
            }
            if (covering_node == NULL) {
                covering_node = last_node->getLargestInSubTree();
            }

            if (!recursive) {   // in non recursive mode, we are done.
...

If you agree that this one is simpler and more readable, then we also
don't need to make predecessor() public either, and I'd suggest
keeping it private.

[snip]

I don't follow what you mean by exploit here. Do you mean not have the tree-of-tree structure? TreeNodeRRset gets its name directly from the node in the tree-of-trees.

What I meant (re-reading it now, I admit the original comment wasn't
clear) is that I'd rather define and use something like
TreeNode* DomainTree::largestNode(), which returns the largest node
in the tree of tree. In the case of the NSEC3 tree, this should be
the same as "largest in subtree" (in the subtree of NSEC3 hash names).

I admit the encapsulation of DomainTree isn't really clean, and we
sometimes allow the applications to use the knowledge of its internal
structure. But when possible, I'd like to hide such details as it's a
"tree of tree" as much as possible, and have applications use
DomainTree just like some kind of abstract container. The concept
of "getting the largest entry" can exist for any type of container
that has the notion of ordering, but getLargestInSubTree looks too
much rely on specific internal details of it. It might be less likely
that we'll fundamentally revise the internal details (e.g., like
changing tree to hash), but if we have choices for some purpose that
have comparable costs, I'd like to choose the one that can hide
internal details more.

I follow what you're trying to do, i.e., keep DomainTree encapsulated as much as possible. In the first case, I used predecessor() because previousNode() can exit the sub-tree and go to the upper level. I've changed it so that it uses the code above now.

Replying to jinmei:

Replying to 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":

    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.

Such a check was added after the last review (to see if the tree is empty).

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

The getSubTreeRoot() was a utility used by both getUpperNode() and getLargestInSubTree(). I've made these functions private now.

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

This was changed because size_t is the appropriate type for memory sizes and memory-sized counters (like number of items of something, unless there is known to be a smaller hard-limit). Before, there was a mix of int and unsigned int. I've changed it to uint32_t now.

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

I've renamed it to findZoneData() and made it non-virtual, non-const, and added a comment about its use in testing.

  • This now-removed comment makes me nervous
    -    // 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?

That comment was added by me in #2108 as I thought we'd be going towards a different interface for findZone(). But we kept the same interface, so the comment was removed.

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

It was used in the tests inside rrsetsCheck(). The TreeNodeRRset tests were missed, but this needed to work for the zone finder tests to pass. I'll skip adding tests in this branch as #2219 has them and we'll get them when these merge. Same for adding getTTL() too, but that was indeed tested.

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

This whole thing is gone now, as NSEC3Hash is now used and faked_nsec3.cc is mostly untouched except addition of some data.

  • should this be removed?
        // This needs to be updated and checked when implementing #2118
    

Gone now.

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

These are tested for length now.

  • I don't understand this comment:
            // We assume that rrsig has already been checked to match rrset
            // by the caller.
    

It was an error in pasting some RdataSet::create() code from elsewhere. Removed.

  • I don't think this test really does what it claims:
        // 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.

I have checked this. It indeed throws in the tree node count test at the top of findNSEC3(). If that test is removed, it asserts at the bottom of that method.

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

Updated.

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

Added.

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.

This hack was removed and replaced with another NSEC3Hash::create() variant which is much cleaner. I wish I had thought of doing this before.

The in-memory implementation does not keep any function pointers now, as the code in NSEC3Hash itself has a way to provide fake hash calculation.

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.

This code is gone now due to the use of NSEC3Hash.

zone_finder.cc

In addition to other points that were already discussed, I've noticed
the same patter like this repeated:

            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:

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:

            ConstRRsetPtr closest = createNSEC3RRset(node, getClass());

Done. :)

comment:15 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by jinmei

Replying to jinmei:

The revised version looks pretty good. I have a few more, relatively
minor things to discuss though.

domaintree.h

  • A minor point: I don't think DomainTreeNodeChain::level_count_ has to be uint32_t because the structure size concern doesn't apply here.
  • I'd note largestNode() never throws.
  • I intended to suggest making some comments about why we use uint32_t for node_count_, but the revised code doesn't have it. I've added my suggested text at commit 40d4887.
  • I have sympathy with keeping the now unused getLargestInSubTree() and its tests (within if 0), but I'd like to clean them up unless we know we need it soon (and I don't think that's the case). If we really worry about the risk of reimplementing it from the scratch if and when we see the need for it, I'd just leave one line comment that we previously implemented it experimentally and removed it as we didn't see the need for it at that time.

memory_client.cc

  • what's the purpose of the changes at e05f1d87c3764033b920fa8a227cfd05dc3ca3b8?
        [2218] Call setSigned(true) when NSEC3Data is set
    
    The commit log doesn't explain the reason, and no test fails even without the changes. It's probably more about the semantics of "is signed" in general - from a quick look, we don't seem to have a consistent interpretation between the zone finder, (document on possible implementation of) zone data, and your implementation of the memory client. Maybe it's a subject of a separate ticket. But in any event, if you want to introduce a change like this (that changes the state of the data), I think there should be some corresponding test case that would fail without the change. So we must either provide a test or defer the change to the later ticket.
  • is there a test for this?
    +            if ((salt_len > 0) &&
                     (std::memcmp(&salt_data_2[0], salt_data, salt_len) != 0)) {
                     isc_throw(AddError,
                               "NSEC3 with inconsistent parameters: " <<
    ...
    +                    if ((salt_len > 0) &&
    
    the code is obviously correct, but in general it's better to confirm it by tests.

memory_client.h, cc

  • these comments seem to have to be updated:
        /// This derived version of the method never throws an exception.
        /// For other details see \c DataSourceClient::findZone().
    
    at least the first line isn't correct (it's not "derived" any more). I'm also not sure if the second line makes sense.
  • I'd suggest changing the return type of findZoneData() to const ZoneData*. That's sufficient for the tests (which seem to be the only use case for it), and will help when we eventually work on #2292 (where we should also change zone_data to a const pointer). I'm attaching a diff to implement it.
  • memory_client.h shouldn't rely on datasrc/zonetable.h, which will be gone when we complete the migration (the diff in the previous bullet makes this cleanup).

domaintree_unittest.cc

  • largestNode test should at least include a test case for an empty tree. I guess there could be some more test cases ideally, but I don't have a specific idea, so it's probably okay with just adding the empty case.

zone_finder_unittest.cc

  • does addZoneData() need everything for the NSEC3 case? It seems to be almost a duplicate of the main code, which is not generally good. For example, if the test data aren't expected to contain inconsistent NSEC3 param, it's probably better just to skip the check.

zone_finder.cc

I have a few more suggestions

  • InMemoryZoneFinder::getOrigin() is expensive and should better be avoided whenever possible. It looks like possible to use the label sequence of the origin node and use LabelSequence? based interfaces in both cases.
  • it should be possible to define salt and hash outside of the loop
        for (unsigned int labels = qlabels; labels >= olabels; --labels) {
            const std::vector<uint8_t> salt(nsec3_data->getSaltData(),
                                            (nsec3_data->getSaltData() +
                                             nsec3_data->getSaltLen()));
            const std::auto_ptr<NSEC3Hash> hash
                (NSEC3Hash::create(nsec3_data->hashalg,
                                   nsec3_data->iterations,
                                   salt));
    
  • If we adopt the suggested interface for NSEC3Hash::create (below), we could omit the construction of salt:
            const std::auto_ptr<NSEC3Hash> hash
                (NSEC3Hash::create(nsec3_data->hashalg,
                                   nsec3_data->iterations,
                                   nsec3_data->getSaltData(),
                                   nsec3_data->getSaltDataLen()));
    
  • boost::scoped_ptr would be slightly better than auto_ptr because we don't have to copy hash.

nsec3hash.h,cc

  • A minor, and possibly controversial point: for the new signature of factory, it might be better to take const uint8_t* and size_t instead of vector<uint8_t>, considering this is a relatively lower level primitive when performance is important. If we require a vector, the caller needs to prepare for it, which might be expensive depending on the context (see above).
  • I'd describe the parameters more specifically for this
        /// \brief Factory method of NSECHash from args.
        ///
        /// This is similar to the other version, but uses the arguments
        /// passed as the parameters for hash calculation.
        static NSEC3Hash* create(uint8_t algorithm, uint16_t iterations,
                                 const std::vector<uint8_t>& salt);
    
    because it's not as obvious as the other two cases.

Changed 7 years ago by jinmei

comment:16 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:17 in reply to: ↑ 15 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

Replying to jinmei:

The revised version looks pretty good. I have a few more, relatively
minor things to discuss though.

domaintree.h

  • A minor point: I don't think DomainTreeNodeChain::level_count_ has to be uint32_t because the structure size concern doesn't apply here.

Changed back to size_t. :)

  • I'd note largestNode() never throws.

Added.

  • I intended to suggest making some comments about why we use uint32_t for node_count_, but the revised code doesn't have it. I've added my suggested text at commit 40d4887.

Looks good.

  • I have sympathy with keeping the now unused getLargestInSubTree() and its tests (within if 0), but I'd like to clean them up unless we know we need it soon (and I don't think that's the case). If we really worry about the risk of reimplementing it from the scratch if and when we see the need for it, I'd just leave one line comment that we previously implemented it experimentally and removed it as we didn't see the need for it at that time.

Removed.

memory_client.cc

  • what's the purpose of the changes at e05f1d87c3764033b920fa8a227cfd05dc3ca3b8?
        [2218] Call setSigned(true) when NSEC3Data is set
    
    The commit log doesn't explain the reason, and no test fails even without the changes. It's probably more about the semantics of "is signed" in general - from a quick look, we don't seem to have a consistent interpretation between the zone finder, (document on possible implementation of) zone data, and your implementation of the memory client. Maybe it's a subject of a separate ticket. But in any event, if you want to introduce a change like this (that changes the state of the data), I think there should be some corresponding test case that would fail without the change. So we must either provide a test or defer the change to the later ticket.

From a reading of ZoneData?'s api doc, I thought setSigned() would also be called with true whenever NSEC3 data existed. I've added tests to assert this.

  • is there a test for this?
    +            if ((salt_len > 0) &&
                     (std::memcmp(&salt_data_2[0], salt_data, salt_len) != 0)) {
                     isc_throw(AddError,
                               "NSEC3 with inconsistent parameters: " <<
    ...
    +                    if ((salt_len > 0) &&
    
    the code is obviously correct, but in general it's better to confirm it by tests.

Added.

memory_client.h, cc

  • these comments seem to have to be updated:
        /// This derived version of the method never throws an exception.
        /// For other details see \c DataSourceClient::findZone().
    
    at least the first line isn't correct (it's not "derived" any more). I'm also not sure if the second line makes sense.

Updated.

  • I'd suggest changing the return type of findZoneData() to const ZoneData*. That's sufficient for the tests (which seem to be the only use case for it), and will help when we eventually work on #2292 (where we should also change zone_data to a const pointer). I'm attaching a diff to implement it.

The patch looks good. I've committed it and also fixed some more new tests for it.

  • memory_client.h shouldn't rely on datasrc/zonetable.h, which will be gone when we complete the migration (the diff in the previous bullet makes this cleanup).

Nod.

domaintree_unittest.cc

  • largestNode test should at least include a test case for an empty tree. I guess there could be some more test cases ideally, but I don't have a specific idea, so it's probably okay with just adding the empty case.

Added.

zone_finder_unittest.cc

  • does addZoneData() need everything for the NSEC3 case? It seems to be almost a duplicate of the main code, which is not generally good. For example, if the test data aren't expected to contain inconsistent NSEC3 param, it's probably better just to skip the check.

Let's keep this for now as the worst thing it does is check more. :) I want to finish up the other loose ends and merge this.

zone_finder.cc

I have a few more suggestions

  • InMemoryZoneFinder::getOrigin() is expensive and should better be avoided whenever possible. It looks like possible to use the label sequence of the origin node and use LabelSequence? based interfaces in both cases.

Done. I've also made it use the previous-chain relative find() which was added in a recent bug to search relative to the origin node.

  • it should be possible to define salt and hash outside of the loop
        for (unsigned int labels = qlabels; labels >= olabels; --labels) {
            const std::vector<uint8_t> salt(nsec3_data->getSaltData(),
                                            (nsec3_data->getSaltData() +
                                             nsec3_data->getSaltLen()));
            const std::auto_ptr<NSEC3Hash> hash
                (NSEC3Hash::create(nsec3_data->hashalg,
                                   nsec3_data->iterations,
                                   salt));
    
  • If we adopt the suggested interface for NSEC3Hash::create (below), we could omit the construction of salt:
            const std::auto_ptr<NSEC3Hash> hash
                (NSEC3Hash::create(nsec3_data->hashalg,
                                   nsec3_data->iterations,
                                   nsec3_data->getSaltData(),
                                   nsec3_data->getSaltDataLen()));
    

Done.

  • boost::scoped_ptr would be slightly better than auto_ptr because we don't have to copy hash.

Done.

nsec3hash.h,cc

  • A minor, and possibly controversial point: for the new signature of factory, it might be better to take const uint8_t* and size_t instead of vector<uint8_t>, considering this is a relatively lower level primitive when performance is important. If we require a vector, the caller needs to prepare for it, which might be expensive depending on the context (see above).

It's been changed now. NSEC3Hash was internally refactored for it.

  • I'd describe the parameters more specifically for this
        /// \brief Factory method of NSECHash from args.
        ///
        /// This is similar to the other version, but uses the arguments
        /// passed as the parameters for hash calculation.
        static NSEC3Hash* create(uint8_t algorithm, uint16_t iterations,
                                 const std::vector<uint8_t>& salt);
    
    because it's not as obvious as the other two cases.

Done.

comment:18 in reply to: ↑ 17 ; follow-ups: Changed 7 years ago by jinmei

I think this ticket can now be closed.

As you know I've already merged this branch with other remaining
branches for the feature. So please simply remove the branch on
closing the ticket.

As discussed on jabber, I made some simplification and style cleanups.

Responses to some specific points in the previous discussion follow:

Replying to muks:

memory_client.cc

  • what's the purpose of the changes at e05f1d87c3764033b920fa8a227cfd05dc3ca3b8?
        [2218] Call setSigned(true) when NSEC3Data is set
    
    The commit log doesn't explain the reason, and no test fails even without the changes. It's probably more about the semantics of "is signed" in general - from a quick look, we don't seem to have a consistent interpretation between the zone finder, (document on possible implementation of) zone data, and your implementation of the memory client. Maybe it's a subject of a separate ticket. But in any event, if you want to introduce a change like this (that changes the state of the data), I think there should be some corresponding test case that would fail without the change. So we must either provide a test or defer the change to the later ticket.

From a reading of ZoneData?'s api doc, I thought setSigned() would also be called with true whenever NSEC3 data existed. I've added tests to assert this.

I'm not sure where in the zone data doc you had the impression, but
according to this:

/// This class does not actually relate the status of signed-or-not to
/// any of its other attributes; it's up to the application how to set or
/// use this status and maintain it in a reasonable way.  One possible
/// definition is to set this status if and only if the zone has a
/// DNSKEY RR at the zone origin (which is BIND 9's definition of signed
/// zone).  When the application adopts this definition, it's the
/// application's responsibility to keep the status consistent with the
/// actual existence or non-existence of a DNSKEY RR.
....
/// Note that even though \c isNSEC3Signed() being true should indicate
/// \c isSigned() is true too in practice, the interfaces do not
/// automatically ensure that, so we'd need to check both conditions
/// explicitly.  And, in fact, if we adopt the above definition of
/// \c isSigned(), it's possible that a zone has a complete set of NSEC3
/// RRs but no DNSKEY (although it's effectively a broken zone unless we
/// support incremental signing).

It rather suggests the concept of signed-or-non-signed is independent
from the existence of NSEC/NSEC3. (but maybe the second paragraph gave
you your impression?).

The relationship and clear definition of "signed" were left open
partially on purpose - I expected when we support incremental
signing or migration between NSEC and NSEC3, it will be possible that
the zone wouldn't be considered "signed" but still have NSEC or NSEC3
records. That's the basic rationale of separating the concept of
"NSEC signed" and "zone is signed". In that sense, it doesn't really
match the original vague intent if we mark the zone "signed" just
because the zone has NSEC3 (or NSEC3PARAM).

But, anyway, at the moment this won't cause any difference in
observable server behavior as we only support complete, full loading
of zones.

So, for the purpose of this ticket I'm okay with the current code.
We'll need a separate cleanup ticket though.

zone_finder_unittest.cc

  • does addZoneData() need everything for the NSEC3 case? It seems to be almost a duplicate of the main code, which is not generally good. For example, if the test data aren't expected to contain inconsistent NSEC3 param, it's probably better just to skip the check.

Let's keep this for now as the worst thing it does is check more. :) I want to finish up the other loose ends and merge this.

Okay, can you open a ticket for that?

zone_finder.cc

I have a few more suggestions

  • InMemoryZoneFinder::getOrigin() is expensive and should better be avoided whenever possible. It looks like possible to use the label sequence of the origin node and use LabelSequence? based interfaces in both cases.

Done. I've also made it use the previous-chain relative find() which was added in a recent bug to search relative to the origin node.

Okay. In the case of the NSEC3 tree I'm not sure how much it buys or
whether it's even really faster (I'm assuming you did it as a kind of
performance optimization), but I don't oppose to the change itself.

Looking into the code further, I noticed another possibility of
optimization:

    for (unsigned int labels = qlabels; labels >= olabels; --labels) {
        const Name& hname = (labels == qlabels ?
                             name : name.split(qlabels - labels, labels));
        const std::string hlabel = hash->calculate(hname);

if we extend NSEC3Hash() so it can take a LabelSequence (must be
absolute), we should be able to avoid the expensive Name::split().
But that's definitely beyond the scope of this ticket. And,
calculate() is probably the most significant bottleneck in this
context anyway, so other optimizations may be marginal.

nsec3hash.h,cc

  • A minor, and possibly controversial point: for the new signature of factory, it might be better to take const uint8_t* and size_t instead of vector<uint8_t>, considering this is a relatively lower level primitive when performance is important. If we require a vector, the caller needs to prepare for it, which might be expensive depending on the context (see above).

It's been changed now. NSEC3Hash was internally refactored for it.

I'd keep the internal vector and copy the given data + size into it,
but I don't oppose to your version.

comment:19 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:20 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 10.75

comment:21 in reply to: ↑ 18 Changed 7 years ago by jinmei

Replying to jinmei:

I think this ticket can now be closed.

As you know I've already merged this branch with other remaining
branches for the feature. So please simply remove the branch on
closing the ticket.

Sorry, I deleted the "trac2218" branch in the remote (shared)
repository when I actually intended to delete trac2219. The
actual working branch for this ticket, trac2218_2, is still there and
should be deleted.

comment:22 in reply to: ↑ 18 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

It rather suggests the concept of signed-or-non-signed is independent
from the existence of NSEC/NSEC3. (but maybe the second paragraph gave
you your impression?).

Yes it was that paragraph.

The relationship and clear definition of "signed" were left open
partially on purpose - I expected when we support incremental
signing or migration between NSEC and NSEC3, it will be possible that
the zone wouldn't be considered "signed" but still have NSEC or NSEC3
records. That's the basic rationale of separating the concept of
"NSEC signed" and "zone is signed". In that sense, it doesn't really
match the original vague intent if we mark the zone "signed" just
because the zone has NSEC3 (or NSEC3PARAM).

But, anyway, at the moment this won't cause any difference in
observable server behavior as we only support complete, full loading
of zones.

So, for the purpose of this ticket I'm okay with the current code.
We'll need a separate cleanup ticket though.

Let's clean it up when we support incremental signing. We will notice it anyway then.

zone_finder_unittest.cc

  • does addZoneData() need everything for the NSEC3 case? It seems to be almost a duplicate of the main code, which is not generally good. For example, if the test data aren't expected to contain inconsistent NSEC3 param, it's probably better just to skip the check.

Let's keep this for now as the worst thing it does is check more. :) I want to finish up the other loose ends and merge this.

Okay, can you open a ticket for that?

I've fixed it in branch trac2218_3. Please review it and I'll close this bug after that.

zone_finder.cc

I have a few more suggestions

  • InMemoryZoneFinder::getOrigin() is expensive and should better be avoided whenever possible. It looks like possible to use the label sequence of the origin node and use LabelSequence? based interfaces in both cases.

Done. I've also made it use the previous-chain relative find() which was added in a recent bug to search relative to the origin node.

Okay. In the case of the NSEC3 tree I'm not sure how much it buys or
whether it's even really faster (I'm assuming you did it as a kind of
performance optimization), but I don't oppose to the change itself.

Looking into the code further, I noticed another possibility of
optimization:

    for (unsigned int labels = qlabels; labels >= olabels; --labels) {
        const Name& hname = (labels == qlabels ?
                             name : name.split(qlabels - labels, labels));
        const std::string hlabel = hash->calculate(hname);

if we extend NSEC3Hash() so it can take a LabelSequence (must be
absolute), we should be able to avoid the expensive Name::split().
But that's definitely beyond the scope of this ticket. And,
calculate() is probably the most significant bottleneck in this
context anyway, so other optimizations may be marginal.

This would need additional methods in LabelSequence (that are in Name such as downcase()) to work on a copy of the passed label sequence inside calculate(). LabelSequence? is also written to assume that its internal data is const, so we may have to create a Name copy instead (there's no inexpensive way to do this from LabelSequence).

nsec3hash.h,cc

  • A minor, and possibly controversial point: for the new signature of factory, it might be better to take const uint8_t* and size_t instead of vector<uint8_t>, considering this is a relatively lower level primitive when performance is important. If we require a vector, the caller needs to prepare for it, which might be expensive depending on the context (see above).

It's been changed now. NSEC3Hash was internally refactored for it.

I'd keep the internal vector and copy the given data + size into it,
but I don't oppose to your version.

comment:23 in reply to: ↑ 22 Changed 7 years ago by jinmei

Replying to muks:

But, anyway, at the moment this won't cause any difference in
observable server behavior as we only support complete, full loading
of zones.

So, for the purpose of this ticket I'm okay with the current code.
We'll need a separate cleanup ticket though.

Let's clean it up when we support incremental signing. We will notice it anyway then.

BTW I've created a ticket: #2295.

Let's keep this for now as the worst thing it does is check more. :) I want to finish up the other loose ends and merge this.

Okay, can you open a ticket for that?

I've fixed it in branch trac2218_3. Please review it and I'll close this bug after that.

In the branch you seem to have updated addZoneDataNSEC3() while I
actually mentioned addZoneData(). In fact, the same sense of comments
would apply to both, so the changes to addZoneDataNSEC3() are fine,
but we need to handle addZoneData().

But before going further, I'd like to point out that more
fundamentally: Do we need to implement the loading
logic in the test again? Almost everything in addZoneDataNSEC3() and
addZoneData() seem to be more of less a copy of the adding/loading
logic in memory_client.cc. Can't we use the existing
InMemoryClient::add() interface?

comment:24 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:25 Changed 7 years ago by muks

I suspect InMemoryClient::add() uses some other member variables from the InMemoryClient object. For use from the tests, it should be made a static function ideally to work on the passed ZoneData& alone. I'll see about this in #2268.

comment:26 follow-up: Changed 7 years ago by muks

The duplicate loading code has been removed as a part of ticket #2268. When that bug is merged to master, I'll close this bug too.

comment:27 in reply to: ↑ 26 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to muks:

The duplicate loading code has been removed as a part of ticket #2268. When that bug is merged to master, I'll close this bug too.

This is now done as a part of bug #2268. Resolving this bug as fixed.

Note: See TracTickets for help on using tickets.