Opened 7 years ago

Closed 7 years ago

#2106 closed task (fixed)

allow RBTree::find to take LabelSequence

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

Description

i.e., define this:

template <typename T>
template <typename CBARG>
typename RBTree<T>::Result
RBTree<T>::find(const dns::LabelSequence& target_labels,
                RBNode<T>** target,
                RBTreeNodeChain<T>& node_path,
                bool (*callback)(const RBNode<T>&, CBARG),
                CBARG callback_arg) const

Actually, the current version (taking Name) will then be a trivial
wrapper for this version.

We'll need this if we encode RDATA names as name+offset data, instead
of pointers to tree nodes for additional record handling (otherwise
we'll need to do expensive construction of Name object).

Subtickets

Change History (11)

comment:1 Changed 7 years ago by shane

  • Estimated Difficulty changed from 0 to 3

comment:2 Changed 7 years ago by jelte

  • Milestone set to Sprint-20120731

comment:3 Changed 7 years ago by muks

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

Picking

comment:4 Changed 7 years ago by muks

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

Up for review.

comment:5 follow-up: Changed 7 years ago by jinmei

Basically looks okay, but I have some minor points.

  • doxygen docs for the Name and LabelSequence versions are mostly duplicate. I'd unify them to minimize redundant text. Or, perhaps we might even obsolete the name version if it's really only used from InMemoryZoneFinder.
  • Likewise, can we share the code for callbackXXX tests? They'd better be unified both for minimizing dups and for making both cases more complete.
  • In the find() implementation, maybe we rename target_labels to something like target_labels_orig and keep the original target_labels as it was? Especially because due to renaming it to _copy there's one 'too long' line:
            node_path.last_comparison_ = target_labels_copy.compare(node->getLabels());
    

comment:6 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to muks

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

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

Basically looks okay, but I have some minor points.

  • doxygen docs for the Name and LabelSequence versions are mostly duplicate. I'd unify them to minimize redundant text. Or, perhaps we might even obsolete the name version if it's really only used from InMemoryZoneFinder.

The Name method has been removed and the memory datasrc now uses the LabelSequence? variant.

  • Likewise, can we share the code for callbackXXX tests? They'd better be unified both for minimizing dups and for making both cases more complete.

Done. :)

  • In the find() implementation, maybe we rename target_labels to something like target_labels_orig and keep the original target_labels as it was? Especially because due to renaming it to _copy there's one 'too long' line:
            node_path.last_comparison_ = target_labels_copy.compare(node->getLabels());
    

Done. :)

comment:8 follow-up: Changed 7 years ago by jinmei

It's basically okay for merge (once #2091 is merged).

I've made some updates to the documentation with some trivial style
fixes, including

  • explanation of why we use label sequence in the most detailed version of find()
  • restore the removed note that this version is intended for in-memory data source implementation. I didn't see any reason for removing it due to the change of this branch
  • assuming we'll soon merge #2092 and #2093, updated the doc about parent tree chaining.

A couple of more minor things:

  • I'd omit defining a variable result:
        //const LabelSequence ls(name); => ls is only used here
        const DomainTree::Result result =
            domains_.find(LabelSequence(name), &node, node_path, cutCallback,
                          &state);
    
    but that may be a matter of taste.
  • it seems callbackName and callbackLabelSequence can be merged:
    TEST_F(RBTreeTest, callback) {
        const Name n1("sub.callback.example");
        const Name n2("callback.example");
        const LabelSequence ls1(n1);
        const LabelSequence ls2(n2);
    
        performCallbackTest(rbtree, mem_sgmt_, n1, n2); // with Names
        performCallbackTest(rbtree, mem_sgmt_, ls1, ls2); // with LabelSequences
    }
    
    but I'd leave it to you, too.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:10 in reply to: ↑ 8 Changed 7 years ago by muks

Replying to jinmei:

A couple of more minor things:

  • I'd omit defining a variable result:
        //const LabelSequence ls(name); => ls is only used here
        const DomainTree::Result result =
            domains_.find(LabelSequence(name), &node, node_path, cutCallback,
                          &state);
    
    but that may be a matter of taste.

This has been implemented now.

  • it seems callbackName and callbackLabelSequence can be merged:
    TEST_F(RBTreeTest, callback) {
        const Name n1("sub.callback.example");
        const Name n2("callback.example");
        const LabelSequence ls1(n1);
        const LabelSequence ls2(n2);
    
        performCallbackTest(rbtree, mem_sgmt_, n1, n2); // with Names
        performCallbackTest(rbtree, mem_sgmt_, ls1, ls2); // with LabelSequences
    }
    
    but I'd leave it to you, too.

It can't be, as performCallbackTest() modifies the data (the second test needs the fixture to be re-created).

comment:11 Changed 7 years ago by muks

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

Merged to master in commit 8fa723bfd4e41dca1c398dfc1350b419d50facea:

* 72f8c0c (origin/trac2106) [2106] Rewrite code to avoid using a variable
* 866fcdb [2106] updated documentation
* a539dcc [2106] Change variable naming slightly
* e50cd96 [2106] Remove Name variant of a RBTree::find() method
* 97873bf [2106] Allow RBTree::find() to take LabelSequence

Resolving as fixed. Thank you for the reviews Jinmei.

Note: See TracTickets for help on using tickets.