Opened 5 years ago

Closed 5 years ago

#2108 closed task (fixed)

redefine in-memory zone load()

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20120918
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: scalable inmemory
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

First, we should move it from ZoneFinder (finder is supposed to be
read-only accessor, so it's been very awkward that it has a "load"
method). This will eventually need to be changed again with scalable
loading, but for now my suggestion is to move it to
InMemoryDataSourceClient.

I also suggest making sure both an RRset and its RRSIG (when signed)
are given at the time of constructing RdataSet?. Due to the encoding
restriction, if this doesn't hold we need to destroy the old data and
reconstruct everything.

The load function/method will insert a tree node (if necessary),
allocate/construct RdataSet?, and add it to the node. Except for the
previous paragraph, this should be essentially an internal
refactoring.

The current additional-section shorcut won't be necessary; that part
shouldn't be ported.

Depends on #2107.

Subtickets

Change History (9)

comment:1 Changed 5 years ago by shane

  • Estimated Difficulty changed from 0 to 6

comment:2 Changed 5 years ago by jelte

  • Milestone set to Sprint-20120821

comment:3 Changed 5 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking

comment:4 Changed 5 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

This bug is up for review in the trac2108_3 branch.

Notes to the reviewer:

  • The branch is best reviewed as a single diff, and not in pieces as there are too many commits. It will be easier to review as a single diff. Please study the interface in memory_client.h first.
  • There are changes to other classes too which were required for the InMemoryClient implementation. Changes to these should be straightforward to review.
  • Note that there's no concept of ZoneFinder inside the InMemoryClient implementation (which is different from the one in the datasrc directory.
  • A findZone2() method has been introduced as DataSourceClient::findZone()'s declaration has a type different from what we'll be using in datasrc::memory.
Last edited 5 years ago by muks (previous) (diff)

comment:5 follow-up: Changed 5 years ago by jelte

  • Owner changed from UnAssigned to muks

zone_table.[h|cc]:

in setZoneData(), should we differentiate between whether a node was not found and whether a node was found, but its old data was empty? (behaviour is different but no way to tell from the caller's side)

memory_messages.mes is not sorted (we have a tool for that: tools/reorder_message_file.py)

memory_client.h:

  • the comment "(but there's a plan to use database backends as a source of the in memory data)" seems out of date :)
  • for some reason, the getFileName() documentation somehow made it seem as if the Client can only load and represent one zone. I don't think the comment needs to change fundamentally, but I would probably add something like 'of the zone with the given Name' somewhere in the beginning, and change tha last part from 'hasn't loaded any file' to 'has not loaded the zone' (do we expect this to be a normal scenario btw? perhaps a NoSuchZone? exception could also be in order, esp. if setZoneData() would have something like that as well)
  • typo at line 225: 'peristently'
  • Personally, I don't think the pimpl pattern is needed here (and in fact i removed it in my new zonefinder implementation), but no extremely strong opinion atm

On a slightly higher level, is findZone2() needed because we don't (yet) have the ZoneFinder?? (i.e. 2109) I'm not sure why it is needed otherwise (i.e. when all are merged we *should* be able to use the normal findZone(), as i think that should create and return a zondefinder then)

Oh, and cppcheck reports a number of errors, btw :)

memory_client.cc:

  • ZoneData? has a getOriginNode(), in theory this could be passed to the MemoryIterator? constructor (instead of doing a find() there)
  • addFromLoad comment says it throws, but it actually aborts

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

  • Owner changed from muks to jelte

Hi Jelte

Replying to jelte:

zone_table.[h|cc]:

in setZoneData(), should we differentiate between whether a node was not found and whether a node was found, but its old data was empty? (behaviour is different but no way to tell from the caller's side)

I've updated setZoneData() to return a FindResult. It also used to set the data in case of a PARTIALMATCH which was a bug. That has been fixed. Note that I have not added code to make it throw an OutOfZone exception instead, as the result is sufficient to determine it. Our current code never sets out-of-zone data, so there's an assert() added towards that.

memory_messages.mes is not sorted (we have a tool for that: tools/reorder_message_file.py)

Done. :)

memory_client.h:

  • the comment "(but there's a plan to use database backends as a source of the in memory data)" seems out of date :)

Fixed. :)

  • for some reason, the getFileName() documentation somehow made it seem as if the Client can only load and represent one zone. I don't think the comment needs to change fundamentally, but I would probably add something like 'of the zone with the given Name' somewhere in the beginning, and change tha last part from 'hasn't loaded any file' to 'has not loaded the zone' (do we expect this to be a normal scenario btw? perhaps a NoSuchZone? exception could also be in order, esp. if setZoneData() would have something like that as well)

Fixed. client_list.cc is coded up to look for an empty string and do the throwing, but I agree throwing a NoSuchZone exception here is valid. Let's do this at the end of the integration with the client list so that it's not something unexpected.

  • typo at line 225: 'peristently'

Fixed. :)

  • Personally, I don't think the pimpl pattern is needed here (and in fact i removed it in my new zonefinder implementation), but no extremely strong opinion atm

I prefer it this way as otherwise it would clutter memory_client.h with all that which is happening inside memory_client.cc.

On a slightly higher level, is findZone2() needed because we don't (yet) have the ZoneFinder?? (i.e. 2109) I'm not sure why it is needed otherwise (i.e. when all are merged we *should* be able to use the normal findZone(), as i think that should create and return a zondefinder then)

Yes, when we merge these two branches and there is a ZoneFinder available, we can remove findZone2(). Without findZone(), the code wouldn't compile due to a missing virtual function, so a dummy one was added that throws.

Oh, and cppcheck reports a number of errors, btw :)

cppcheck returned one error in the new InMemoryClient work. It was an used variable which is now removed. :)

There are some others from the TreeNodeRRset work. But I had branched this from trac2098 when TreeNodeRRset was still in review. Maybe these issues are now fixed as cppcheck is not complaining on master.

memory_client.cc:

  • ZoneData? has a getOriginNode(), in theory this could be passed to the MemoryIterator? constructor (instead of doing a find() there)

The MemoryIterator uses the node chain returned by find() during nextNode(). I guess we can start with a freshly created chain object as we are starting from the origin, but we'd rely on a DomainTree implementation detail then.

  • addFromLoad comment says it throws, but it actually aborts

I've updated this comment. addFromLoad() used to throw before, but now the called code itself throws. add() should never return anything except SUCCESS, so if it returns anything else, we assert() now. addFromLoad() exists as a thin wrapper around add().

comment:7 follow-up: Changed 5 years ago by jelte

  • Owner changed from jelte to muks

ok, this can be merged, but please verify cppcheck :)

comment:8 in reply to: ↑ 7 Changed 5 years ago by muks

Replying to jelte:

ok, this can be merged, but please verify cppcheck :)

After merging to master, make cppcheck is now clean on my box.

comment:9 Changed 5 years ago by muks

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

Merged to master in commit ee0950bcf935175ebed77a407ad6881e2b482bef:

*   ee0950b Merge branch 'master' into trac2108_3
* 70f25f4 (origin/trac2108_3) [2108] Remove unused variable (reported by cppcheck)
* 6919cf0 [2108] Fix doc comment for addFromLoad()
* 65ce434 [2108] Fix typo
* 7f4c06a [2108] Fix doc comment for InMemoryClient::getFileName()
* 2de58c8 [2108] Update obsolete comment
* 514cf93 [2108] Sort the memory_messages.mes file
* 56afaa4 [2108] Changed proto of ZoneTable::setZoneData() to return a FindResult
* e3f72ac [2108] Remove TODO list
* d976012 [2108] Test for segment leaks due to allocation failures
* f1f2184 [2108] Pass memory segment to InMemoryClient constructor
* dd5485e [2108] Add unittest for load() which accepts an iterator
* 4171ba9 [2108] Wrap lines
* 8716a09 [2108] Add unique prefix to datasrc::memory message IDs
* eb66b3c [2108] Sort zone files in Makefile.am
* 5bd8df4 Revert "[2108] getTTL() is unimplemented in TreeNodeRRset"
* a727e66 [2108] Implement TreeNodeRRset::getTTL()
* f60ea8d [2108] Test iterator with separate_rrs=true
* c531154 [2108] Fix comment
* 72f8b8a [2108] getTTL() is unimplemented in TreeNodeRRset
* a376d4a [2108] Add DNAME+NS tests
* f4ceb19 [2108] Fix zone file comments
* e898d07 [2108] Add CNAME+other tests (which should throw)
* 083e683 [2108] Fix context check when adding a CNAME record
* 3f0eb8f [2108] If there's an exception, clear last_rrset_
* 41eff48 [2108] Check that loading a zone containing NSEC3 names with invalid number of labels throws
* 2d47ae9 [2108] Check that loading a zone with wildcard NSEC3 names throws an exception
* 46286a6 [2108] Fix RRType in last commit comment
* 15bdb89 [2108] Check that loading a zone with wildcard DNAME names throws an exception
* 86d8c64 [2108] Check that loading a zone with wildcard NS names throws an exception
* 5a70502 [2108] Add tests for out of zone names
* 0a9b4b2 [2108] Rename tests
* caf572e [2108] NSEC3/NSEC3PARAM RRs with multiple RDATA should throw
* f476f78 [2108] Add load tests for NSEC3 signed zones
* 22add5b [2108] Multiple DNAME RRs should throw
* f0be7a0 [2108] Multiple CNAME RRs should throw
* 26abb28 [2108] result::EXIST is no longer returned by add()
* b9c8992 [2108] Add tests for duplicate types in load()
* d4026ad [2108] Delete trailing whitespace
* cf9ebfb [2108] Untabify code
* 6be43cd [2108] Add checks on reloaded zone data
* cc5d465 [2108] Add zone counter checks when empty zone file is loaded
* 0f8bc28 [2108] Untabify code
* 11fbb68 [2108] Loading an empty zone without even an SOA for the origin should throw
* 35057f3 [2108] Wrap code
* 1814133 [2108] Add reload test
* 1fe06c9 [2108] Increment zone counter only when zone doesn't already exist
* 330f8ec [2108] Add more checks to data returned by findZone2()
* 2dce3cb [2108] Add comment
* 79e4f78 [2108] Add some more checks in MemoryClientTest.findZone2
* e1f7b3f [2108] Add findZone2() check
* 97cd696 [2108] Check that findZone() derived from DataSourceClient throws
* c080ae5 [2108] Add another item to TODO list
* 02c01e5 [2108] Test when add() is called with an empty rdata RRset
* 6ed1c01 [2108] Test when add() is called with a null RRset
* cf353d1 [2108] Untabify code
* c326d49 [2108] Add test for RRSIGs with mixed covered types
* fb45c7e [2108] In add(), if RRset has attached RRSIGs, add them
* bff38dc [2108] Add another TODO comment
* fb5704f [2108] Wrap lines
* 7a3b73c [2108] Remove some unused variables
* 105b19f [2108] Add test for multiple RRSIGs for a single covered RRset
* 37ef5a7 [2108] Fix covered type in exception message
* 723fcfb [2108] Test when RRSIG doesn't match the type it is supposed to cover
* 365c642 [2108] Update build for testdata directory
* 1af7d12 [2108] Test when RRSIG doesn't match the name it is supposed to cover
* b5efb67 [2108] Test when RRSIG doesn't follow its covered RRset
* 222eefb [2108] Test loading a non-existent zone file
* 1e1882f [2108] Add load() test for a full and correct zone
* 4f132f4 [2108] Test some types of errors in zone files don't cause leaks
* 94670ca [2108] Use the test memory segment in InMemoryClient unittests
* 4da836d [2108] Add tests for getFileName()
* e9d6cf4 [2108] Add a small list of items to do
* 92850a8 [2108] Add another InMemoryClient::add() test
* d06f564 [2108] When adding RRsets, ensure that any last rrset was flushed
* f8b8177 [2108] Test that adding an RRset to a non-existent zone throws
* 91ab8fe [2108] Check that iterating past the end throws an exception
* 64b3b91 [2108] Check that getSOA() on the iterator throws
* 753931a [2108] Check RRsets returned by the iterator
* 32c4d7f [2108] Move iterator.h include to the header file
* ea0b948 [2108] Minor fix to the last comment
* 9158d6d [2108] Add some comments to tests
* 90b10c0 [2108] Add two more InMemoryClient unittests
* 21ff0c5 [2108] Add getIterator() test for existing zone
* 42cd408 [2108] Untabify code
* b3310ca [2108] Rename test
* 0523c84 [2108] Add a basic InMemoryClient::load() test
* a879e6b [2108] Add some includes useful in memory client tests
* 0f42cb5 [2108] Initialize logging in tests
* d5e7b61 [2108] Untabify code
* d475b8b [2108] Remove use of a scoped_ptr
* 12a6f52 [2108] getJournalReader() must throw isc::NotImplemented
* 63eba85 [2108] getUpdater() must throw isc::NotImplemented
* 7e46f0a [2108] Check for "." zone instead
* 38db4f8 [2108] Add testcase for non-existent zone's iterator
* 9d1a6c8 [2108] Add the outline of a memory client test file
* 31a1014 [2108] Add the ZoneIterator for the memory client
* cc90d4e [2108] Add another typedef in ZoneData class
* f573cd7 [2108] Add log messages related parts in lib/datasrc/memory
* 8b1a6f2 [master] Update .gitignore
* d074626 [2108] Save RRsets and call RdataSet::create() with their corresponding RRSIGs
* 372c18d [2108] Add underscore to member variable
* 0aa28c0 [2108] Make FindResult return non-const ZoneData
* 558aff9 [2108] Fix NSEC3PARAM variable name
* 0c4fd30 [2108] Fix namespace of node flags
* c13e1fb [2108] Port contextCheck() to new API
* 65282c5 [2108] Remove use of getClass() which doesn't exist in the Impl class
* 1628d5f [2108] Port NSEC3PARAM check to the new design
* a10c87f [2108] Rename member variable to contain underscore
* 36acee5 [2108] Update InMemoryClientImpl::add() to use the new design
* 1c1627f [2108] Remove name of unused argument
* ad45b4e [2108] Remove an excess argument
* c396c0b [2108] Fix use of member variable
* 99db6f4 [2108] Add a findZone() variant temporarily to derive from DataSourceClient
* 9be7fe0 [2108] Remove unused includes
* 120561d [2108] Remove what's left of RBNodeRRset
* 429f2e7 [2108] Remove additionals handling code
* 896c0de [2108] Update comment
* f262336 [2108] Swap the new ZoneData in place when loading a zone
* 061fbc1 [2108] Add ZoneTable::setZoneData()
* d1c78d4 [2108] Fix addWildCards() further
* bba5b86 [2108] Port addWildcards() to the new design
* bd22127 [2108] Save file name during load() and implement getFileName()
* ae9a4ac [2108] Add an InMemoryClient implementation
* 57a9640 [2108] Add initial interface for InMemoryClient

Resolving as fixed. Thank you for the reviews Jelte.

Note: See TracTickets for help on using tickets.