BIND 10 #451: MemoryZone::load()

[BIND 10 Development]

#451: MemoryZone::load()
------------------------------+---------------------------------------------
      Reporter:  jinmei       |        Owner:  jinmei               
          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 vorner):

  * owner:  vorner => jinmei


Comment:

 As #447 is in trunk, I rebased it and prepared, so now it is in
 branches/trac451. Last three commits (r4039, r4044, r4045) are new since
 you revied it.

 Replying to [comment:2 jinmei]:
 > '''RBTree::swap()'''
 > Okay, although it doesn't have a test.

 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?

 > '''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.

 > '''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.

 Yes, it is done, with the simplification (mentioned below).

 > '''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).

 >  - 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.

 > '''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());
 > }}}

 I added a test for it.

 > '''Other points'''
 >  - There are some style issues regarding the space and '&'/'*':
 > {{{
 >         void load(const string &filename) {
 > ...
 >         MemoryZone *zone_;
 > }}}

 Hmm, right. Should be fixed.

 >  - You may also want to update MemoryDatasourceConfig::build() (and
 >    its tests) so that it will actually load the master file.

 Done

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