BIND 10 #2835: add interface to get properties of datasrc clients from ClientList

[BIND 10 Development]

#2835: add interface to get properties of datasrc clients from ClientList
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130402
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:11 vorner]:

 > > - In any case, I'm afraid "MSS" could be misunderstood especially
 > >   because it's commonly used to mean "maximum segment size".  I'd name
 > >   them a bit more specific at the risk of sounding verbose.
 >
 > I must admit I never heard that, but changing the prefix.

 It's a TCP terminology.  See, e.g., RFC 6691.  (So with a proper
 context it might not be that confusing, but if you only see an enum
 value in the source code that could still cause confusion).

 > > - `name` can be a reference here:
 > > {{{#!cpp
 > >             const string name(name_elem ? name_elem->stringValue() :
 type);
 > > }}}
 >
 > I found out that many people get confused about this. So, I think we
 should use it only in case there's some real performance benefit.
 Comparing what other we do in the configuration and how often it is run, I
 think this is not the place to sacrifice clarity to performance.

 Fair enough (and in this specific case this would be quite cheap for
 many implementations of std::string).

 (Complete) comments on the revised branch follow:

 '''changelog'''

 The fact that data source can be named may be worth noting, but I'd
 leave it to you.

 '''datasrc.spec.pre.in'''

 - IIRC, an optional item must have a default (otherwise, e.g., bindctl
   doesn't work correctly with it), although I may be wrong - the
   concept of "optional" has always confused me.

 '''client_list.h'''

 - `MemorySegmentState`: did you intentionally name SEGMENT_MAPPED that
   way, instead of INUSE?  I'm not necessarily opposed to it, but in
   that case I suggest clarifying "mapped" has a higher level meaning
   that includes locally allocated memory (i.e., not only for memory
   mapped with mmap(2), etc).

 - `MemorySegmentState`: I'd clarify the type description a bit more,
   e.g.:
 {{{#!cpp
 /// Describes the status in which the segment for in-memory cache of
 /// given data source is.
 enum MemorySegmentState {
 }}}
   clarifying it's for the "in-memory cache".

 - `MemorySegmentType`: I'm not sure if we define them here.  In my
   image specific details of segment types are private to memory
   segment (and corresponding `ZoneTableSegment`) implementations, and
   the `ClientList` class uses it transparently.  So, for example, we
   might allow introducing another type of memory segment as a plugin
   in future.  For that reason my original intent is to define the type
   in a semantics-free type, such as a plan std::string.

 - `DataSourceStatus`: regarding the description, I guess we might use
   it for things not related to memory segments, e.g., some specific
   operation on a specified data source (specified by its name).  So
   I'd revise it a bit, e.g., as follows:
 {{{#!cpp
 /// This indicates the status a data soure is in. It is currently used
 with
 /// segment and cache management, to discover the data sources that need
 /// external mapping or local loading.  In future, it may be extended for
 /// other purposes such as performing an operation on the data source
 /// of a given name.
 }}}
 - `DataSourceStatus`: in doxygen descriptions, "current" may (now)
   make much sense because the state of this object never changes after
   construction (or I misunderstand what "current" means here).

 - I'd clarify this can throw std::bad_alloc (in rare case) and
   otherwise exception free:
 {{{#!cpp
     /// \brief Get status information of all internal data sources.
     ///
     /// Get a DataSourceStatus for current state of each data source
 client
     /// in this list.
     std::vector<DataSourceStatus> getStatus() const;
 }}}

 - `DataSourceStatus::getSegmentType()`: I guess `InvalidOperation`
   might be better if we want to throw here.

 '''client_list_unittest'''

 - I guess the comment should be updated:
 {{{#!cpp
 // Check the status holds data and can change the segment state
 TEST(DataSourceStatus, status) {
 }}}
   Segment state cannot be changed any more, and in any case there's
   no such test here.  And, the second chunk of test is not really
   included in what is commented here.

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