BIND 10 #418: new query logic for in memory data source (1.5)

[BIND 10 Development]

#418: new query logic for in memory data source (1.5)
---------------------------+------------------------------------------------
      Reporter:  jinmei    |        Owner:  jelte                
          Type:  task      |       Status:  reviewing            
      Priority:  major     |    Milestone:  y2 12 month milestone
     Component:  b10-auth  |   Resolution:                       
      Keywords:            |    Sensitive:  0                    
Estimatedhours:  0.0       |        Hours:  0                    
      Billable:  1         |   Totalhours:  0                    
      Internal:  0         |  
---------------------------+------------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => jelte


Comment:

 Replying to [comment:4 jelte]:

 > Should the bare pointer in ZoneTable::!FindResult not be a
 !ConstZonePtr?
 >
 It's intentional and documented.  See the "notes to developer" part of the
 !ZoneTable class description.

 I personally think we tend to overuse shared pointers even when the
 ownership of the objects don't have to be (or are not even supposed to be)
 shared.  I'm afraid it make us less careful about the ownership
 relationships among classes in design.  On the other hand, using shared
 pointers make code safer in general, and in that sense there's nothing
 wrong with them except their slightly higher overhead (and I don't worry
 about the overhead at the moment).  So, this is actually an open point.  I
 don't mind changing it to shared pointers if you like that approach.

 > I do wonder about the general direction this is heading though; for
 instance this change implies a new mandatory subclass for each datasource.
 One or two of those may not be a problem, but it feels like a slippery
 slope.
 >
 I don't yet have a clear image about how the zone class would be used when
 everything is integrated, but I can think of two possibilities:
  - if the underlying database such as some variant of SQL doesn't have an
 explicit representation of zones (as part of public interface), we can
 probably use a "default" zone class that simply encapsulates the
 corresponding data source and calls its find() (or whatever we finally
 come up with) method
  - some data source may want to specialize it by inheritance as an
 optimization.  for example, in the current schema design of the sqlite3
 data source, its zone (derived) class would contain the information of the
 "zone ID".

 I'm not sure if this answers your comment, though...(whether or not it
 makes sense to you) does it?

 > If the Zone class is only meant to be a specific subclass from
 AbstractZone for the in-memory datasource, I would suggest we rename it,
 to better reflect that.
 >
 > If it is possible that we can provide a default class for 'general'
 datasources that don't need specific features, that's what Zone should be
 (if not, I think we should consider renaming AbstractZone to Zone
 actually, to make calling code more intuitive; Zone::NXDOMAIN vs
 AbstractZone::NXDOMAIN); and then recheck the shared_ptr typedefs, their
 names don't match what they encapsulate right now.
 >
 Yes, I know the inconsistency between the shared pointer typedef and the
 class name, and I was aware it's awkward to have the AbstractZone:: prefix
 in application code.  On the other hand, clearly naming an abstract base
 class has some advantage, e.g., it can emphasize the point that it's
 expected to define interfaces only and all methods should be pure virtual.
 So, the current implementation is a compromise on the tradeoff.

 I'm okay with renaming AbstractZone to Zone in preferring consistency,
 though. (And in any case the current Zone class will be renamed to
 something like InMemoryZone anyway).  Taking into account the background,
 would you like to make that change?

 > Oh and a tiny comment on a comment; I'd say that the FindResult
 struct/class is all state, not no state (comment at the definition of the
 first one zonetable.h line 72) :)
 >
 By "no internal state" I meant once constructed it's never changed.  But
 as you pointed out "no state" would not really be accurate for that
 meaning.  How about this?

 {{{
     /// This is a simple value class whose internal state never changes,
 so for
     /// convenience we allow the applications to refer to the members
 }}}

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