BIND 10 #1331: Extend the ZoneUpdater class to support adding diffs

[BIND 10 Development]

#1331: Extend the ZoneUpdater class to support adding diffs
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111122
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:         |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''general/design'''

 - I personally think we should notify the caller of an explicit
   failure if `journaling` is true while the underlying data source
   doesn't support it.

   We should eventually support the capabilities (there's a ticket),
   and once it's supported the application will be expected to first
   check the capability of the data source and set 'journaling'
   accordingly.  (otherwise, things like dynamic DNS won't work
   correctly if, for example, the underlying data source is volatile
   like writable in-memory without a persistent journal system; the
   application would at least rely on the journal to be stored in a
   persistent storage and return a successful response to a dynamic DNS
   update request once `updater.commit()` succeeds.  If the underlying
   data source actually doesn't support it and simply ignore the
   mismatch, an unexpected thing will happen due to e.g. power
   failure).

   With this scenario in mind, the application shouldn't even try to
   use journaling unless it knows it's supported, and, if some strange
   reason `addRRset()` or `deleteRRset()` fails due to diff handling
   failure even the capability check suggests it's supported, the
   application should actually know that.   At the very least I believe
   we should log the event here
 {{{
             // Don't fail if the backend can't store them
             catch(const isc::NotImplemented&) {}
 }}}
   Silently discarding an internal failure is generally not a good
   practice.

 - I'm not sure if we should allow this flexibility: "When the
   journaling is true, it *might* require that the following update be
   formated as IXFR" (as documented in client.h), and I'd rather
   suggest it be required without may/might.  Unless/until we have a
   smart data source backend that can automatically figure out the
   correct ordering internally *and* the application selectively wants
   to use that particular data source, this "may" will simply increase
   the possibility of giving a false sense of flexibility to
   applications, which subsequently leads to bugs.

 - As for changelog, I personally don't have an opinion, but
   considering Jeremy wanted to have a changelog entry for #1329
   (which is even more invisible for most users/developers than this
   one), I suspect he'd also want to have it for this task.

 '''zone.h'''

 Maybe in the class description, I suggest adding a reference to
 `DataSourceClient::getUpdater` for the meaning of "IXFR sequence".

 '''database.cc'''

 I've not found any substantial issues (except for things related to
 the higher level design).  There are some things to discuss though:

 - while quite straightforward, I'm afraid that addRRset() and
   deleteRRset() are now getting bigger and decreasing readability.
   I propose something like the attached diff.  This patch actually
   contains one (unrelated) bug fix: reject an empty RRset at an
   earlier point of validation; in the previous code if an empty RRset
   is passed and rejected by other validation check, `RRset::toText()`
   would cause another disruption.

 - 'rdata::' could be omitted here:
 {{{
             serial_ =
                 dynamic_cast<const
 rdata::generic::SOA&>(it->getCurrent()).
                 getSerial();
 }}}
  (there are two such cases)

 - need a space after `catch`:
 {{{
             // Don't fail if the backend can't store them
             catch(const isc::NotImplemented&) {}
 }}}
   - also, I've been noticing style inconsistency on whether to place
     catch in the same line of the closing brace for try.
   - but in this particular case I'd rather not catch the exception here
     in the first place as already commented.

 - `Unexpected` doesn't seem to be an appropriate exception here:
 {{{
         isc_throw(isc::Unexpected, "Update sequence not complete");
 }}}
   This exception was generally intended to be used for internal
   unexpected events.  In this case this is basically an application
   bug, so we should use something else.  Maybe abusing `BadValue` even
   though it's not about the parameter, or introduce something like
   `BadOperation`, or some new data source specific exception.

 '''database_unittest.cc'''

 - I've fixed a build error on my system (actually I was surprised it
   compiled on other systems) and pushed it.  commit 6da32ea.

 - This code seems to be a bit cryptic to me:
 {{{
         for (size_t i(0); i < DatabaseAccessor::DIFF_PARAM_COUNT; ++ i) {
             data_equal = data_equal && (data_[i] == other.data_[i]);
         }
 }}}
   While I generally love the elegance of a concise statement, this
   particular one made me use more brain cycle to understand, perhaps
   because it's used in the context of traditional procedural style.
   While it may look dumb, I'd rather prefer something like this:
 {{{
         for (size_t i(0); i < DatabaseAccessor::DIFF_PARAM_COUNT; ++ i) {
             if (data_[i] != other.data_[i]) {
                 data_equal = false;
                 // we could even break here;
             }
         }
 }}}
   If you really want to keep the original style, I won't fight against
   it further, but I'd at least suggest adding redundant parentheses
   for readability:
 {{{
             data_equal = (data_equal && (data_[i] == other.data_[i]));
 }}}

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