BIND 10 #2218: update InMemoryZoneFinder::findNSEC3 using memory-efficient version
BIND 10 Development
do-not-reply at isc.org
Tue Sep 25 21:37:58 UTC 2012
#2218: update InMemoryZoneFinder::findNSEC3 using memory-efficient version
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
jinmei | Status: reviewing
Type: task | Milestone:
Priority: | Sprint-20121009
medium | Resolution:
Component: data | Sensitive: 0
source | Sub-Project: DNS
Keywords: | Estimated Difficulty: 5
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
scalable inmemory |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:12 jinmei]:
The revised version looks pretty good. I have a few more, relatively
minor things to discuss though.
'''domaintree.h'''
- A minor point: I don't think DomainTreeNodeChain::level_count_ has
to be uint32_t because the structure size concern doesn't apply
here.
- I'd note largestNode() never throws.
- I intended to suggest making some comments about why we use uint32_t
for node_count_, but the revised code doesn't have it. I've added
my suggested text at commit 40d4887.
- I have sympathy with keeping the now unused getLargestInSubTree() and
its tests (within if 0), but I'd like to clean them up unless we know
we need it soon (and I don't think that's the case). If we really
worry about the risk of reimplementing it from the scratch if and
when we see the need for it, I'd just leave one line comment that we
previously implemented it experimentally and removed it as we didn't
see the need for it at that time.
'''memory_client.cc'''
- what's the purpose of the changes at
e05f1d87c3764033b920fa8a227cfd05dc3ca3b8?
{{{
[2218] Call setSigned(true) when NSEC3Data is set
}}}
The commit log doesn't explain the reason, and no test fails even
without the changes. It's probably more about the semantics of "is
signed" in general - from a quick look, we don't seem to have a
consistent
interpretation between the zone finder, (document on possible
implementation of) zone data, and your implementation of the memory
client. Maybe it's a subject of a separate ticket. But in any
event, if you want to introduce a change like this (that changes the
state of the data), I think there should be some corresponding test case
that
would fail without the change. So we must either provide a test or
defer the change to the later ticket.
- is there a test for this?
{{{
+ if ((salt_len > 0) &&
(std::memcmp(&salt_data_2[0], salt_data, salt_len) != 0))
{
isc_throw(AddError,
"NSEC3 with inconsistent parameters: " <<
...
+ if ((salt_len > 0) &&
}}}
the code is obviously correct, but in general it's better to confirm
it by tests.
'''memory_client.h, cc'''
- these comments seem to have to be updated:
{{{#!cpp
/// This derived version of the method never throws an exception.
/// For other details see \c DataSourceClient::findZone().
}}}
at least the first line isn't correct (it's not "derived" any
more). I'm also not sure if the second line makes sense.
- I'd suggest changing the return type of findZoneData() to
`const ZoneData*`. That's sufficient for the tests (which seem to
be the only use case for it), and will help when we eventually work
on #2292 (where we should also change zone_data to a const pointer).
I'm attaching a diff to implement it.
- memory_client.h shouldn't rely on datasrc/zonetable.h, which will be
gone when we complete the migration (the diff in the previous bullet
makes this cleanup).
'''domaintree_unittest.cc'''
- largestNode test should at least include a test case for an empty
tree. I guess there could be some more test cases ideally, but I
don't have a specific idea, so it's probably okay with just adding
the empty case.
'''zone_finder_unittest.cc'''
- does addZoneData() need everything for the NSEC3 case? It seems to
be almost a duplicate of the main code, which is not generally good.
For example, if the test data aren't expected to contain
inconsistent NSEC3 param, it's probably better just to skip the
check.
'''zone_finder.cc'''
I have a few more suggestions
- InMemoryZoneFinder::getOrigin() is expensive and should better be
avoided whenever possible. It looks like possible to use the label
sequence of the origin node and use LabelSequence based interfaces
in both cases.
- it should be possible to define salt and hash outside of the loop
{{{#!cpp
for (unsigned int labels = qlabels; labels >= olabels; --labels) {
const std::vector<uint8_t> salt(nsec3_data->getSaltData(),
(nsec3_data->getSaltData() +
nsec3_data->getSaltLen()));
const std::auto_ptr<NSEC3Hash> hash
(NSEC3Hash::create(nsec3_data->hashalg,
nsec3_data->iterations,
salt));
}}}
- If we adopt the suggested interface for NSEC3Hash::create (below),
we could omit the construction of salt:
{{{#!cpp
const std::auto_ptr<NSEC3Hash> hash
(NSEC3Hash::create(nsec3_data->hashalg,
nsec3_data->iterations,
nsec3_data->getSaltData(),
nsec3_data->getSaltDataLen()));
}}}
- boost::scoped_ptr would be slightly better than auto_ptr because we
don't have to copy hash.
'''nsec3hash.h,cc'''
- A minor, and possibly controversial point: for the new signature of
factory, it might be better to take `const uint8_t*` and `size_t`
instead of `vector<uint8_t>`, considering this is a relatively lower
level primitive when performance is important. If we require a
vector, the caller needs to prepare for it, which might be expensive
depending on the context (see above).
- I'd describe the parameters more specifically for this
{{{#!cpp
/// \brief Factory method of NSECHash from args.
///
/// This is similar to the other version, but uses the arguments
/// passed as the parameters for hash calculation.
static NSEC3Hash* create(uint8_t algorithm, uint16_t iterations,
const std::vector<uint8_t>& salt);
}}}
because it's not as obvious as the other two cases.
--
Ticket URL: <http://bind10.isc.org/ticket/2218#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list