BIND 10 #451: MemoryZone::load()

[BIND 10 Development]

#451: MemoryZone::load()
------------------------------+---------------------------------------------
      Reporter:  jinmei       |        Owner:  vorner               
          Type:  task         |       Status:  reviewing            
      Priority:  major        |    Milestone:  y2 12 month milestone
     Component:  data source  |   Resolution:                       
      Keywords:               |    Sensitive:  0                    
Estimatedhours:  0.0          |        Hours:  0                    
      Billable:  1            |   Totalhours:  0                    
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => vorner
  * status:  new => reviewing


Comment:

 Replying to [comment:1 vorner]:
 > It uses MemoryZone::add(), so I can't rebase the branch yet (it wouldn't
 compile) and prepare the actual branch. But yes, it is not directly
 related to it, so if you want to read the changes before the rebase, go
 ahead, they should be r3926-r3931 in trac441.
 >
 I believe I've reviewed all relevants part of branches/trac441.  Here are
 review results:

 '''RBTree::swap()'''
 Okay, although it doesn't have a test.

 '''About the general design of loading'''
 It's not compatible with the assumption of auth/configureAuthServer():
 with this implementation MemoryDatasourceConfig::build() would build
 new zone data and replace the old one, while the auth config stuff
 assumes the "replace" part is done by
 MemoryDatasourceConfig::commit().

 Right now, it won't cause a problem because the MemoryDatasourceConfig
 class always build all zones from the scratch (so the "old zone" is
 always empty) and replaces the entire data source.  But if we want to
 support selectively updating zones in a memory data source (we'll
 actually want that eventually) it will become a real problem.

 One way to be compatible with the config assumption would be to
 separate the "build (or load)" and "replace" methods, and have
 MemoryDatasourceConfig::build/commit() call
 MemoryDataSrc::build/replace() respectively.  (see also the comment
 about the number of swaps for MemoryZoneImpl::Load below)

 '''MemoryZone::load()'''
  - Maybe a matter of style (and the compiler would probably generate
    the same code), but it could be a bit simplified without using a
    temporary variable:
 {{{
     MemoryZoneImpl::Load(this).load(filename);
 }}}

 '''MemoryZoneImpl::add()'''
  - as we discussed in a separate ticket, we may want to use assert()
    instead of AssertError, and/or not use getRelativeName()
    optimization.  Note also that in that case some part of the
    documentation about exceptions may also be updated.

 '''MemoryZoneImpl::Load class'''
  - It seems to me we can reduce the number of swaps (only at the
    "finally" point) by building the domain tree separately during the
    loading.  And it can be quite easy to do that if we pass
    impl_->domains_ to MemoryZoneImpl::add():
 {{{
     result::Result add(DomainTree& domains, const ConstRRsetPtr& rrset) {
     // and also replace "domains_" with "domains"
 }}}
   and perform some trivial adjustments.  With this change we can
   implement the separation between "load" (or prepare) and "replace"
   as commented above.  It will also help if and when we want to
   support incremental load (we'll probably want to do that to reload a
   large zone in a single-process, single-thread server program).
  - add(): the current behavior of EXIST is okay for now due to the
    assumption of the acceptable zone file, but we'll eventually want to
    support "merging" the interleaved RRsets.
  - also about the EXIST case: I'm not sure if we use the dns module
    exception.  But as noted in the previous bullet, considering this
    would be a short term restriction, it probably won't matter much.
    (This is just a comment and I'd leave the decision to you.)

 '''MemoryZoneTest::load()'''
 These tests look okay, but this part of Load::add() isn't covered:
 {{{
                 case result::EXIST:
                     isc_throw(dns::MasterLoadError, "Duplicate rrset: " <<
                         set->toText());
 }}}

 '''Other points'''
  - There are some style issues regarding the space and '&'/'*':
 {{{
         void load(const string &filename) {
 ...
         MemoryZone *zone_;
 }}}
  - You may also want to update MemoryDatasourceConfig::build() (and
    its tests) so that it will actually load the master file.

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