Opened 8 years ago

Closed 8 years ago

#2098 closed task (complete)

Define and Implement `TreeNodeRRset` class

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20120904
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: 6 Add Hours to Ticket: 0
Total Hours: 16.35 Internal?: no

Description (last modified by jinmei)

(Since some people seem to react to the term "RBTree" knee-jerk, this
is given a more generic name. For the same reason we might want to
make it a templated class).

This is a revised version of RBNodeRRset. It will take RBNode (or
"TreeNode") and RdataSet and hold pointers to them. All of its
public interfaces are implemented through the underlying tree node and
RdataSet. In the initial version, we could omit many of them "not
implemented": all setXXX can be skipped. Even getRdataIterator() can
be skipped. Some can be inefficient for now (e.g. getName()).

The only important part is the toWire() method. It will use the
encoded version of iterator, and render names using Label Sequence.

To be more specific about what should be done in this task: my
suggestion is to implement:

  • constructor
  • getClass
  • getType
  • getName (in an inefficient way)
  • toWire (could be only for the renderer)
  • empty implementation of getRRsig (just return a null pointer)

Everything else can be "throw not_implemented" for now.

This will leave one non trivial open issue: how to render signed
RRset. I have some suggestion regarding this, but I'd
defer it for a separate discussion and for a later sprint (it's #2163).

Depends on #2097 and (at least) #2093.

Subtickets

Change History (19)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by shane

  • Estimated Difficulty changed from 0 to 6

comment:3 Changed 8 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 8 years ago by jelte

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

comment:5 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:6 Changed 8 years ago by jelte

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

comment:7 Changed 8 years ago by jelte

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

comment:8 Changed 8 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:9 Changed 8 years ago by jinmei

trac2098 is ready for review.

I rebased the branch on to a more recent version of master in the
middle of the development, but I believe the branch tree is clean.
(Just in case it's necessary) the effective branch point is 949291a.

I needed to do a couple of preparation changes:

  • move the label sequence version of messagerenderer toWire() to the base class (b350749)
  • port getAbsoluteLabels() from the rbtree class to doamintree (068ecfa).

I believe these should be a straightforward copy-and-adjust.

Other than these, this branch essentially consists of newly introduced
files: treenode_rrset.h, .cc, and their tests. I'm afraid the code
size is a bit large, but I believe the implementation is pretty easy
to understand (even if some specific way of it may be subject to
discussions).

Finally, I added a simple small benchmark test under benchmarks. I
think it's only needed a lightweight review, or, if the branch looks
already too big, I'm okay with excluding it for now.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:11 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:12 Changed 8 years ago by jinmei

I forgot to mention this: I implemented some more methods than
the ones listed in the original ticket description. I found them
necessary in the auth query processing.

comment:13 follow-up: Changed 8 years ago by vorner

Hello

There's just one substantial thing. Cppcheck complains:

src/lib/datasrc/memory/benchmarks/rrset_render_bench.cc:153: check_fail: Missing bounds check for extra iterator increment in loop. (warning,StlMissingComparison)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::origin_node_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::www_node_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::wildcard_node_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::ns_rdataset_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::dname_rdataset_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::a_rdataset_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::aaaa_rdataset_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::rrsig_only_rdataset_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:44: check_fail: Member variable 'TreeNodeRRsetTest::wildcard_rdataset_' is not initialized in the constructor. (warning,uninitMemberVar)
make: *** [cppcheck] Error 1

Then there's some amount of details. There are few empty functions, with comments they are not used (all three of them are writeName(const LabelSequence&, const bool in some context). I think it would be safer to throw from them, to make sure they are really not used.

The test TreeNodeRRsetTest::toWire contains several scopes. Why are the variables like const TreeNodeRRset rrset1(rrclass_, www_node_, a_rdataset_, true); numbered?

The checkToWireResult and checkTruncationResult are very similar functions. Would it be possible to merge them to one?

The conversion of name to stored data and then back when a label sequence is needed seems slightly overcomplicated (name->label squence->data->label sequence->possibly name). But I don't have a concrete idea how to simplify it.

    const LabelSequence labels(realname);
    const size_t labels_storangelen = labels.getSerializedLength();
    realname_buf_ = new uint8_t[labels_storangelen];
    labels.serialize(realname_buf_, labels_storangelen);

I needed to read the code of the tested function and re-read the comment to understand it. I think it is quite confusing just by itself:

        // Same for the owner name (note: in practice this method would be
        // called for rrsets at different nodes, so we check that condition
        // first).  Note also that based on the basic assumption of the
        // ZoneTree, if the nodes are different their RR classes must be
        // different.

comment:14 Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

comment:15 in reply to: ↑ 13 Changed 8 years ago by jinmei

Thanks for the review.

Replying to vorner:

There's just one substantial thing. Cppcheck complains:

Ah, thanks for checking. Fixed it. There is one other unrelated
cppcheck regression that has been fixed in master. I've cherry-picked
that fix too.

Then there's some amount of details. There are few empty functions, with
comments they are not used (all three of them are `writeName(const
LabelSequence?&, const bool` in some context). I think it would be safer to
throw from them, to make sure they are really not used.

Okay, done (in some cases I chose assert() rather than exception).

The test TreeNodeRRsetTest::toWire contains several scopes. Why are the
variables like `const TreeNodeRRset rrset1(rrclass_, www_node_,
a_rdataset_, true);` numbered?

Ah, these are leftovers from an older versions where there was no
scope. Now removed the numbers.

The checkToWireResult and checkTruncationResult are very similar
functions. Would it be possible to merge them to one?

The difficulty is, maybe it's premature though, that I wanted to make
checkToWireResult generic so that it could be used for OutputBuffer
too (as commented). So unifying them would introduce its own
complexity. But I tried to unify the two cases while keeping the
ability to support OutputBuffer. And, in fact, I added one test
case so we actually test it with an OutputBuffer (which results in
exception).

The conversion of name to stored data and then back when a label sequence
is needed seems slightly overcomplicated (name->label squence->data->label
sequence->possibly name). But I don't have a concrete idea how to simplify
it.

    const LabelSequence labels(realname);
    const size_t labels_storangelen = labels.getSerializedLength();
    realname_buf_ = new uint8_t[labels_storangelen];
    labels.serialize(realname_buf_, labels_storangelen);

Yeah, I know. On looking it further, I realized it would be much
simpler if we just copy the given Name object. And, it would simplify
other parts of the code. Originally I used a plain old array,
expecting we might want to optimize it using a pre allocated space
from some pool. But, it may be premature, and serialize() or
restoring from it is also not super cheap anyway, so I revised the
code by copying the name object and holding the copy.

I needed to read the code of the tested function and re-read the comment
to understand it. I think it is quite confusing just by itself:

        // Same for the owner name (note: in practice this method would be
        // called for rrsets at different nodes, so we check that
condition
        // first).  Note also that based on the basic assumption of the
        // ZoneTree, if the nodes are different their RR classes must be
        // different.

Is it that the code comment is not very understandable? If the more
formal documentation in the header file of the isSameKind() method
is understandable, maybe we should just refer to it and simplify the
in-code comment (I wrote the latter first in development, and then
thought this would require some detailed explanation and did it in the
header). If your concern is something else, or the header description
is not easy to understand either, please explain what's the concern.

Finally, I made a few additional changes (19e4b20 through 49aabed). I
believe the commit log and change explain them.

comment:16 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:17 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 2.85

Hello

I noticed one more thing. The class contains several pointers that are taken
care of manually. Bad things could happen if the class is copied. I don't think
we want to ever copy it, so defining our own copy operator and constructor is
probably an overkill. So, should it be derived from boost::non_copyable, just
to make sure?

Otherwise it seems OK to merge.

Thank you

comment:18 in reply to: ↑ 17 Changed 8 years ago by jinmei

Replying to vorner:

we want to ever copy it, so defining our own copy operator and constructor is
probably an overkill. So, should it be derived from boost::non_copyable, just
to make sure?

Okay, done. I've added documentation about the rationale too.

Assuming you didn't mean to review that part, I've merged the branch,
and am now closing the ticket.

comment:19 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 2.85 to 16.35
Note: See TracTickets for help on using tickets.