Opened 9 years ago

Closed 9 years ago

#451 closed task (complete)

MemoryZone::load()

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: y2 12 month milestone
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

It looks like this part of branches/trac441 can be reviewed in parallel with others. So I've created a ticket for it.

Subtickets

Change History (7)

comment:1 follow-up: Changed 9 years ago by 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.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner
  • Status changed from new to reviewing

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

comment:3 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to jinmei

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

comment:4 in reply to: ↑ 3 ; follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

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

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by vorner

Replying to jinmei:

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

I put one into config.cc and one into memory_datasource.h (as a doxygen \todo tag).

One minor nit: the opening brace position for add() could be adjusted:

ACK, done.

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

Something like the one I added in r4049?

comment:6 in reply to: ↑ 5 Changed 9 years ago by jinmei

Replying to vorner:

Replying to jinmei:

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

I put one into config.cc and one into memory_datasource.h (as a doxygen \todo tag).

Okay, these look good.

Okay, and I'd add a test case where new_zone->load() throws an exception.

Something like the one I added in r4049?

Ah, yes.

Now it's okay to merge.

comment:7 Changed 9 years ago by vorner

  • Resolution set to complete
  • Status changed from reviewing to closed

It is merged.

Note: See TracTickets for help on using tickets.