Opened 5 years ago

Closed 5 years ago

#2109 closed task (complete)

redefine in-memory zone finder (basic)

Reported by: jinmei Owned by: jelte
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: 7 Add Hours to Ticket: 0
Total Hours: 2.73 Internal?: no

Description (last modified by jinmei)

With #2108, the InMemoryZoneFinder can now concentrate on its
find()-related tasks. It will be constructed with ZoneData,
and will be constructed by revised ZoneTable.

It will still be quite complicated, so it's probably defined in a
separate .cc file.

It probably makes more sense to move ZoneData::findNode() to finder's
dedicated subroutine (in an unnamed namespace - it shouldn't have to
be a member function of the finder).

This task ports the basic part of its find() and findAll(). Skip
wildcard consideration. Also just use the base (default) result
context.

Resulting TreeNodeRRset needs to be created dynamically. We should
probably use an object pool.

Depend on #2098, #2107 and #2108.

Subtickets

Change History (12)

comment:1 Changed 5 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 5 years ago by jinmei

  • Description modified (diff)

comment:3 Changed 5 years ago by shane

  • Estimated Difficulty changed from 0 to 7

comment:4 Changed 5 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:5 Changed 5 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120904

comment:6 Changed 5 years ago by jelte

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

comment:7 Changed 5 years ago by jelte

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

Ready for review! (finally).

The important parts are in the new src/lib/datasrc/memory/zone_finder.[h|cc] files.

It is essentially the same implementation as in the original memory_datasrc, but using the new structures. I did however modify it somewhat (removed now unnecessary pimpl and template constructs, and updated logic as necessary to use the new data structures).

The tests are updated too, they no longer need load(), but add rrs to the ZoneData? structure themselves.

I also ported the general structure for doing wildcards and nsec3 testing, but those are disabled pending #2110 and #2218.

comment:8 Changed 5 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:9 follow-up: Changed 5 years ago by vorner

  • Owner changed from vorner to jelte

Hello

I had some problems with running tests, but they all seem unrelated:

  • Distcheck failed, but as mentioned in jabber, this is already fixed in master.
  • My cppcheck went crazy and vomited like 2 pages of errors. But it is because I upgraded, I need to create a bug ticket about that.

A lot of the code seems to be copy-pasted (mostly TestNSEC3HashCreator and the whole test). Should we unify it somehow or do we expect to drop the old version soon?

Also, should these files be part of some kind of utils library? This way they are compiled twice.

run_unittests_SOURCES += ../../tests/faked_nsec3.h ../../tests/faked_nsec3.cc

The order of these headers should maybe be reversed, so the more local ones are first:

#include <datasrc/data_source.h>
#include <testutils/dnsmessage_test.h>

#include <datasrc/memory/zone_finder.h>
#include <datasrc/memory/rdata_serialization.h>

What is the purpose of commenting this out, when will it be enabled or deleted?

 libb10_datasrc_la_LIBADD += $(top_builddir)/src/lib/cc/libb10-cc.la
-libb10_datasrc_la_LIBADD += memory/libdatasrc_memory.la # convenience library
+#libb10_datasrc_la_LIBADD += memory/libdatasrc_memory.la # convenience library
 libb10_datasrc_la_LIBADD += $(SQLITE_LIBS)

I don't understand this TODO:

/**
 * \brief Test searching.
 * 
 * Check it finds or does not find correctly and does not throw exceptions.
 * \todo This doesn't do any kind of CNAME and so on. If it isn't
 *     directly there, it just tells it doesn't exist.
 */    
void
InMemoryZoneFinderTest::findCheck(ZoneFinder::FindResultFlags expected_flags,
                                  ZoneFinder::FindOptions find_options)

And this sentence is hard to parse for me, it has two verbs:

/// Returns an empty TreeNodeRRsetPtr is either node or rdataset is NULL.

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

  • Owner changed from jelte to vorner

Replying to vorner:

I had some problems with running tests, but they all seem unrelated:

  • Distcheck failed, but as mentioned in jabber, this is already fixed in master.

ack, for completeness, I tried merging with master and doing make distcheck, and the problem is not reintroduced by this branch.

  • My cppcheck went crazy and vomited like 2 pages of errors. But it is because I upgraded, I need to create a bug ticket about that.

Hmm, what version are you at now? (my cppcheck reports clean code, but this looks like we need to upgrade again)

A lot of the code seems to be copy-pasted (mostly TestNSEC3HashCreator and the whole test). Should we unify it somehow or do we expect to drop the old version soon?

I expect all memory_datasrc unit tests to be dropped in or right after 2219 is done.

Additionally, for this branch this code wasn't even needed, but I put it in and made sure it compiled to make life easier for the person doing #2218 (which at the time I thought would be me).

as of our new policy, I should add a comment that this is expected to be finalized and/or cleaned up in #2218 (where we know exactly what we can use of it) and #2219 (to remove leftovers, if any). Done

Also, should these files be part of some kind of utils library? This way they are compiled twice.

run_unittests_SOURCES += ../../tests/faked_nsec3.h ../../tests/faked_nsec3.cc

see above :)

The order of these headers should maybe be reversed, so the more local ones are first:

#include <datasrc/data_source.h>
#include <testutils/dnsmessage_test.h>

#include <datasrc/memory/zone_finder.h>
#include <datasrc/memory/rdata_serialization.h>

done

What is the purpose of commenting this out, when will it be enabled or deleted?

 libb10_datasrc_la_LIBADD += $(top_builddir)/src/lib/cc/libb10-cc.la
-libb10_datasrc_la_LIBADD += memory/libdatasrc_memory.la # convenience library
+#libb10_datasrc_la_LIBADD += memory/libdatasrc_memory.la # convenience library
 libb10_datasrc_la_LIBADD += $(SQLITE_LIBS)

Oh right, should've removed it completely; if we need it we have ourselves a circular dependency.

I don't understand this TODO:

/**
 * \brief Test searching.
 * 
 * Check it finds or does not find correctly and does not throw exceptions.
 * \todo This doesn't do any kind of CNAME and so on. If it isn't
 *     directly there, it just tells it doesn't exist.
 */    
void
InMemoryZoneFinderTest::findCheck(ZoneFinder::FindResultFlags expected_flags,
                                  ZoneFinder::FindOptions find_options)

should've been removed already AFAICT; we do do CNAME testing. Removed now.

And this sentence is hard to parse for me, it has two verbs:

/// Returns an empty TreeNodeRRsetPtr is either node or rdataset is NULL.

oops, changed to

/// Returns an empty TreeNodeRRsetPtr if node is NULL or if rdataset is NULL.

comment:11 in reply to: ↑ 10 Changed 5 years ago by vorner

  • Owner changed from vorner to jelte
  • Total Hours changed from 0 to 2.73

Hello

Replying to jelte:

  • My cppcheck went crazy and vomited like 2 pages of errors. But it is because I upgraded, I need to create a bug ticket about that.

Hmm, what version are you at now? (my cppcheck reports clean code, but this looks like we need to upgrade again)

1.56. I have the „luck“ of running gentoo and it seems the cppcheck maintainer is keeping well up to date.

It seems OK now. Please merge.

comment:12 Changed 5 years ago by jelte

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

Thanks! merged, closing ticket

Note: See TracTickets for help on using tickets.