BIND 10 #447: MemoryZone::find()

[BIND 10 Development]

#447: MemoryZone::find()
------------------------------+---------------------------------------------
      Reporter:  vorner       |        Owner:  hanfeng  
          Type:  task         |       Status:  reviewing
      Priority:  major        |    Milestone:           
     Component:  data source  |   Resolution:           
      Keywords:               |    Sensitive:  0        
Estimatedhours:  0.0          |        Hours:  0        
      Billable:  1            |   Totalhours:  0        
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by vorner):

 Replying to [comment:3 jinmei]:
 >  - do we need to include <iostream>?

 Ups, I forgot to remove that one after some debugging. No, it is not
 needed.

 > '''MemoryZoneImpl::add()'''
 >  - this method does not provide strong exception guarantee (if it
 >    throws after insert(), the inserted node will remain).  It may be
 >    okay at least for now, because in the case of exception quite
 >    likely that the entire zone will be given up.  But it may have a
 >    tricky implication if/when we want to support dynamic update using
 >    this implementation (consider the case where we add a new name and
 >    then allocating the data fails with an exception).  so I'd suggest
 >    adding (doxygen) comment about this point.

 I changed find() in a way empty domain is considered nonexistent. That way
 we have strong exception guarantee at the interface level (while the
 internal representation changed, the external behaviour stays the same).

 >  - I'm not sure why we need the tricks with getRelativeName().  As far
 >    as I understand it the rbtree should work without trimming the
 >    origin.  Perhaps it intends to be performance optimization (for
 >    find())?  If so, I suspect the performance gain wouldn't be much
 >    substantial because getting the prefix itself is a quite expensive
 >    operation, and I suspect the possible gain doesn't outweigh the
 >    additional code complexity.

 I wanted it not because of the finding (but traversing the set of labels
 twice, once in ZoneTable, once in the zone itself seems duplicated work),
 but to save some memory. But it probably is marginal.

 Do you think I should remove it in favor of simplicity?

 > '''MemoryZoneImpl::find()'''
 >  - unfortunately, the delegation case cannot be that simple.  (Using
 >    the unittest setting) consider the case we have
 >    ns.subdomain.example.org as RDATA of the subdomain.example.org/NS,
 >    and an A and/or AAAA glue RRs of ns.subdomain.example.org in the
 >    example.org zone.  When we ask for ns.subdomain.example.org we
 >    should return the delegation (unless we really need the glues for
 >    additional section processing), but the current implementation
 >    would return an exact match.  So, my suggestion is to leave the
 >    delegation case open for now and focus on the easier case as
 >    originally intended for this ticket.

 Ah, you're right, I didn't think of that. In that case it doesn't seem to
 be easilly solved by this way, so I removed the wrong code.

 Thanks for the comments.

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