BIND 10 #347: Implement query counters in auth module
BIND 10 Development
do-not-reply at isc.org
Wed Dec 1 13:49:23 UTC 2010
#347: Implement query counters in auth module
------------------------------+---------------------------------------------
Reporter: naokikambe | Owner: naokikambe
Type: enhancement | Status: reviewing
Priority: major | Milestone: y2 12 month milestone
Component: b10-auth | Resolution:
Keywords: | Sensitive: 0
Estimatedhours: 0.0 | Hours: 0
Billable: 1 | Totalhours: 0
Internal: 0 |
------------------------------+---------------------------------------------
Changes (by jinmei):
* owner: jinmei => naokikambe
Comment:
Okay, I've completed my review. Comments below. Giving the ticket back
to Kambe-san.
'''general'''
- The Counter class should have its own tests.
'''auth_srv.h'''
- I don't think this have to be a doxygen comment:
{{{
/// for the typedef of asio_link::IntervalTimer::Callback
}}}
- BTW, if we change the callback scheme as I suggested, we don't
have include asio_link.h/stats.h. (Counter can be a forward
declaration, and shouldn't be here anyway. see below)
- Basically up to you, but this comment seems trivial and I wouldn't
bother to mention it:
{{{
#include <auth/stats.h>
}}}
- getStatsCallback(): see the callback scheme discussion.
- Member variable "counter" breaks the spirit of pimpl. The comment also
does not
seem to make sense to me. Whether it's incremented in a member
function of AuthSrv or AuthSrvImpl, "counter" can be defined in
AuthSrvImpl. The current implementation also is exception unsafe
due to this variable. See below. In any case "counter" should be
hidden in impl_. BTW, to be consistent about naming, I'd rename
it to "counter_".
'''auth_srv.cc'''
- AuthSrv constructor: this is not exception safe. If the allocation
of counter fails memory for impl_ will leak. Due to this, and for
other reasons also, counter should in impl_. BTW: it also doesn't
make sense to me to dynamically allocate Counter, especially
if/when we hide it in .cc. Unless this is optional (in the current
implementation it's not) AuthSrvImpl can simply contain a Counter
object.
- AuthSrv::processMessage(): in which case we can see getProtocol()
be neither UDP nor TCP? In general, I don't like to handle an
impossible case this way. I'd either throw an exception or even
assert() here.
- AuthSrv::getStatsCallback(): the comment seems to me so trivial
that I wouldn't bother to mention it:-)
'''auth.spec.pre.in'''
- as pointed out before, it's not necessarily "b10-stats":
{{{
+ "command_description": "Send statistics data to b10-stats at
once",
}}}
I'd say, e.g., "...to a statistics module at once".
'''asio_link.h'''
- I'd like to see more description for the IntervalTimer class,
including how it generally works and how it should be used (sample
code may help).
- I don't understand this statement:
{{{
+/// Copy of this class is prohibited not to call the callback function
twice.
}}}
Could the callback function be called twice if we allowed copy? (I
don't object to disabling the copy per se). BTW, being pedantic,
we don't copy a "class"; we copy an object (or instance) of a
class.
- IntervalTimer constructor: passing (and exposing as a result)
asio::io_service(&) breaks the intent of this wrapper and is not a
good practice. The only place (as a short-to-middle term
workaround) we can see it is in the cc::Session constructor. No
other code except that within the asio_link wrapper should directly
refer to it, even as a reference. Please update the interface so
that it takes IOService(&) instead of asio::io_service(&). I
believe you can do it with a trivial change.
- IntervalTimer destructor: it should explain it cancels a running
timer. BTW: what happens if we destruct IntervalTimer before
starting a timer?
- setupTimer(): this should explain the expected signature of
cbfunc. See, for example, the asio documentation:
http://think-
async.com/Asio/asio-1.4.5/doc/asio/reference/basic_deadline_timer/async_wait.html
It should also explain it doesn't only register the callback but
also actually starts the timer.
- setupTimer(): should it return a (boolean) value? It doesn't seem
to return no other value than true. If so, it's better to make it
void.
'''asio_link.cc'''
- It'd be helpful if IntervalTimerImpl could have more description.
See other pimpl classes defined in asio_link.cc.
- IntervalTimerImpl::setupTimer(): IMO this use of assert is wrong.
In this case we should use exception, and document the restriction,
and test the case.
- IntervalTimerImpl::callback(): Likewise, this assert seems to be
wrong because, if I understand the code correctly, the application
can (incorrectly) pass an empty functor via setupTimer(). If want
to make sure this doesn't happen, we should require it at the time
of setupTimer(), throw an exception otherwise, document it, and
test it.
'''asio_link_unittest.cc'''
- doTimerTest() doesn't seem to have to be in the ASIOLinkTest
class (because it's small and is only used by a single test).
Placing it in a different place makes the code unreadable.
- doTimerTest(): I'd like to make sure the timer expires with the
given interval, although it could be optional.
- timerCallBack() doesn't have to be a separate function (it can be
embedded in TimerCallBack::operator())
- ASIOLinkTest::startIntervalTimer(): why is itimer allocated
dynamically. It makes (at least in theory) the code exception
unsafe, and we can actually simply avoid the dynamic allocation:
{{{
IntervalTimer itimer(io_service_->get_io_service());
doTimerTest(&itimer);
}}}
btw, you can omit the namespace "asio_link::" because we declare
'using namespace' for it.
- I'd like to see more test cases, including
- Invalid arguments (see the comments on IntervalTimerImpl)
- what happens if we delete the timer
- call setupTimer multiple times, with different arguments
'''auth_srv_unittest.cc'''
- as commented, we need a separate test class for Counter (shouldn't
be "in future", especially because the tests are incomplete - see
below).
- AuthSrvTest::sendStatsWithoutSession() This is not a good unit test
because a human needs to examine the output. Modify the interface
and use EXPECT_xx.
- AuthSrvTest::counter_incUDP/TCP(): these don't do any tests. In
terms of auth server, we should test by sending a "fake" UDP/TCP
message and see if the corresponding counter is incremented. For
the more focused tests of Counter, we directly increment the
counters and check the results. For that matter, we'll need to an
inteface to retrieve the specified counter value in the Counter
class (which would be good anyway). BTW, using _ in the test name
effectively breaks our naming rule. I'd rename it to
inc[rement]CounterUDP, etc. Same comment applies to some other
cases.
- AuthSrvTest::counter_sendStatsWithSession(): this test is
insufficient. We should define an appropriate MockSession, which
intercepts the submitted message, parses and stores it, and then we
should check the submitted values are correct using EXPECT_xxx.
'''main.cc'''
- my_command_handler()
- in this context auth_server shouldn't be NULL. I'd rather assert
it or assume it's valid.
- main()
- I have comments/suggestions about timer callbacks. See above.
'''ChangeLog'''
- Shouldn't Kambe-san be listed as a contributor, too? (Just checking.)
- It would be nice if it could briefly explain how we can see the
collected statistics (via bindctl perhaps).
--
Ticket URL: <http://bind10.isc.org/ticket/347#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list