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


Comment:

 Replying to [comment:3 vorner]:

 > There's RBTreeTest.swap, in rbtree_unittest.cc. Is it possible you
 overlooked it or is there a problem somewhere in the repository and it got
 lost for you?
 >
 Ah, sorry, apparently I overlooked it.

 > > '''About the general design of loading'''
 > > [ ... ]
 > > 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)
 >
 > So, should we just ignore it for now, or should I split it? Or, there's
 another way ‒ providing a swap method for MemoryZone. Or, another one
 would be to load the changed zones „fresh“ and replace them inside the
 MemoryDatasource.
 >
 It's probably okay to move forward with the current code for now.  But
 please leave some comments about the possible need for more incremental
 load support somewhere (either or both in memory_datasource.cc and
 auth/config.cc).

 > > '''MemoryZoneImpl::add()'''
 > > '''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():
 >
 > Yes, good idea. I used it and simplified the code a lot (now it doesn't
 have odd tricks with finally, nor the help class).
 >
 Okay.

 One minor nit: the opening brace position for add() could be adjusted:
 {{{
     result::Result add(const ConstRRsetPtr& rrset, DomainTree* domains) {
 }}}

 > >  - 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.)
 >
 > Well, I somehow expected that the MasterLoader would merge them itself.
 But that might be too strong assumption. I would suggest to leave it as it
 is now, until we know how the MasterLoader will look like when it can load
 any master files.
 >
 Works for me.

 > >  - You may also want to update MemoryDatasourceConfig::build() (and
 > >    its tests) so that it will actually load the master file.
 >
 > Done
 >
 Okay, and I'd add a test case where new_zone->load() throws an exception.

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