BIND 10 #2108: redefine in-memory zone load()

[BIND 10 Development]

#2108: redefine in-memory zone load()
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jelte
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120918
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jelte


Comment:

 Hi Jelte

 Replying to [comment:5 jelte]:
 > zone_table.[h|cc]:
 >
 > in setZoneData(), should we differentiate between whether a node was not
 found and whether a node was found, but its old data was empty? (behaviour
 is different but no way to tell from the caller's side)

 I've updated `setZoneData()` to return a `FindResult`. It also used to set
 the data in case of a PARTIALMATCH which was a bug. That has been fixed.
 Note that I have not added code to make it throw an `OutOfZone` exception
 instead, as the result is sufficient to determine it. Our current code
 never sets out-of-zone data, so there's an `assert()` added towards that.

 > memory_messages.mes is not sorted (we have a tool for that:
 tools/reorder_message_file.py)

 Done. :)

 >
 > memory_client.h:
 >
 > - the comment "(but there's a plan to use database backends as a source
 of the in memory data)" seems out of date :)

 Fixed. :)

 > - for some reason, the getFileName() documentation somehow made it seem
 as if the Client can only load and represent one zone. I don't think the
 comment needs to change fundamentally, but I would probably add something
 like 'of the zone with the given Name' somewhere in the beginning, and
 change tha last part from 'hasn't loaded any file' to 'has not loaded the
 zone' (do we expect this to be a normal scenario btw? perhaps a NoSuchZone
 exception could also be in order, esp. if setZoneData() would have
 something like that as well)

 Fixed. `client_list.cc` is coded up to look for an empty string and do the
 throwing, but I agree throwing a `NoSuchZone` exception here is valid.
 Let's do this at the end of the integration with the client list so that
 it's not something unexpected.

 > - typo at line 225: 'peristently'

 Fixed. :)

 > - Personally, I don't think the pimpl pattern is needed here (and in
 fact i removed it in my new zonefinder implementation), but no extremely
 strong opinion atm

 I prefer it this way as otherwise it would clutter `memory_client.h` with
 all that which is happening inside `memory_client.cc`.

 > On a slightly higher level, is findZone2() needed because we don't (yet)
 have the ZoneFinder? (i.e. 2109) I'm not sure why it is needed otherwise
 (i.e. when all are merged we *should* be able to use the normal
 findZone(), as i think that should create and return a zondefinder then)

 Yes, when we merge these two branches and there is a `ZoneFinder`
 available, we can remove `findZone2()`. Without `findZone()`, the code
 wouldn't compile due to a missing virtual function, so a dummy one was
 added that throws.

 >
 > Oh, and cppcheck reports a number of errors, btw :)
 >

 cppcheck returned one error in the new `InMemoryClient` work. It was an
 used variable which is now removed. :)

 There are some others from the `TreeNodeRRset` work. But I had branched
 this from `trac2098` when `TreeNodeRRset` was still in review. Maybe these
 issues are now fixed as cppcheck is not complaining on master.

 > memory_client.cc:
 >
 > - ZoneData has a getOriginNode(), in theory this could be passed to the
 MemoryIterator constructor (instead of doing a find() there)

 The `MemoryIterator` uses the node chain returned by `find()` during
 `nextNode()`. I guess we can start with a freshly created chain object as
 we are starting from the origin, but we'd rely on a `DomainTree`
 implementation detail then.

 > - addFromLoad comment says it throws, but it actually aborts

 I've updated this comment. `addFromLoad()` used to throw before, but now
 the called code itself throws. `add()` should never return anything except
 SUCCESS, so if it returns anything else, we `assert()` now.
 `addFromLoad()` exists as a thin wrapper around `add()`.

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