BIND 10 #347: Implement query counters in auth module
BIND 10 Development
do-not-reply at isc.org
Wed Dec 1 10:31:10 UTC 2010
#347: Implement query counters in auth module
------------------------------+---------------------------------------------
Reporter: naokikambe | Owner: jinmei
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 |
------------------------------+---------------------------------------------
Comment(by jinmei):
Replying to [comment:4 naokikambe]:
> We finished coding and committing source codes at r3686. It is ready for
reviewing.
>
> Jinmei-san: Could you review it?
>
As explicitly asked and told it was urgent I've given it a higher priority
for a while. Review isn't completed yet, but here's some initial
feedback.
'''general comments'''
- Documentation: It's generally well written, but I'd like to see
description about exceptions, that is, whether each method can
throw an exception, and if yes, when.
- If you don't do propset for $Id$, this line should be removed (in
git, it will be meaningless "random" string, so we should probably
deprecate this convention, at least for newly created files)
'''stats.h'''
- I made some trivial (I think) editorial changes and commit it to
the branch (r3689). Please check them.
- The class name "Counter" sounds like representing a single counter
and counter intuitive.
- I'd like to see more in the class description, including how it
generally works and how we are expected to use it.
- Is this query specific? Except the counter names there doesn't
seem to be anything specific to queries (or in that sense even
specific to an authoritative server). If the intent is to extend
the usage to other types of statistic counters eventually, it
should be described accordingly.
- setStatsSession(): I guess most of the points noted for
AuthSrv::setXfrinSession() should apply.
- getCallback(): I think this method naming is too restrictive, and I'd
suggest renaming it to, e.g., getSubmitFunc(). The reason is below.
Whether it's used as a callback (e.g. from a timer) is a matter of
caller, and the Counter class doesn't have to care about it. All the
responsibility of this method is to return a callable functor object that
when called submit the collected statistics through the session channel
(if provided via setStatsSession()). In fact, b10-auth could always get
the functor and submit the statistics by its own, right? (for example, it
may want to submit the final statistics before it shuts down). In
general, if some public part of a class can be independent of how it can
be used, it's generally better to make that point explicit so that the
class is more cohesive (and less coupled with its users).
For the same reason, I think it's better to change the return type to
some more generic functor type (and I believe it's easy).
And for the same reason, it's okay (and useful) to mention its
intentional usage as a hint, but it should also be described that it can
be used in other ways.
'''stats.cc'''
- why do we need a separate namespace? Just following what asio_link
did? If so, the intent was to make it a separate library belonging
to a separate namespace eventually. Unless the stat module has the
same intent, I'd suggest not having the separate namespace for now
(we should probably need a common "auth" namespace, although that's
a different topic)
- naming: it would be better to use consistent terminology, i.e. if
we need and use a namespace named "statistics", the
header/implementation
files should better be called "statistics.{h,cc}", and vice versa.
I personally prefer (especially in C++) complete words such as
"statistics" rather than the geeky abbreviation like "stats", but
it's generally a matter of taste.
(BTW: the redundancy is also related to the question of why we need
a separate name space.)
- CounterImpl declarations: getCallback() and sendStats()
could/should be const member functions.
- CounterImpl constructor: it seems ad hoc to pass "verbose_mode"
this way. It's also not a good practice in that it ties the
Counter(Impl) class with the AuthSrv class through an implicit
interface (i.e. not via public methods). I can understand if this
is a short term workaround until we have a well defined logging
framework, however. If so, please explicitly note this in the
doxygen comment. If not, and you think this is a good technique,
we need to discuss it because I'd strongly disagree:-)
- CounterImpl constructor: why dynamically allocating counters_? I
don't see the need for it.
- CounterImpl::sendStats()
- CounterImpl::sendStats() try-catch part
- group_sendmsg() (not recv) can also throw. should this be caught
or ignored here? probably it should, because we wouldn't like to
kill b10-auth just due to some communication failure (though it
may be quite rare). In this sense, I suspect the comments should
be positioned closer to the group_recvmsg() call.
- group_recvmsg() can throw (at least) two types of exceptions:
SessionTimeout when it times out; SessionError for other errors,
including when receiving bogus data. I guess we'd like to catch
both for the same reason as that for group_sendmsg().
- The comment and log messages assume that the other end is the
b10-stats program, but interface-wise it's not necessarily correct
(that could be a third-party app that implements the stat API,
right?)
- Considering these, my suggestion of this part is as follows:
{{{
// group_{send,recv}msg() can throw an exception when encountering
// an error, and group_recvmsg() will throw an exception on timeout.
// We don't want to kill the main server just due to this, so we
// handle them here.
try {
const int seq =
stats_session_->group_sendmsg(set_stats, "Stats");
if (verbose_mode_) {
std::cerr << "[b10-auth] send statistics data" << std::endl;
}
// TODO: parse and check response from the statistics recipient;
// in case of b10-stats, currently it just returns empty message.
isc::data::ConstElementPtr env, answer;
stats_session_->group_recvmsg(env, answer, false, seq);
} catch (const isc::data::SessionError& ex) {
if (verbose_mode_) {
std::cerr << "[b10-auth] "
<< "timeout happened while sending statistics data:
"
<< ex.what() << std::endl;
}
} catch (const isc::data::SessionTimeout& ex) {
// catch the exception and do nothing.
if (verbose_mode_) {
std::cerr << "[b10-auth] "
<< "communication error in sending statistics data:
"
<< ex.what() << std::endl;
}
}
}}}
- BTW, even if it doesn't cause timeout and exception, we should
actually not even block in group_recvmsg (and also
group_sendmsg(), which can happen because it's a connected
channel): b10-auth shouldn't stop handling DNS queries even for 1
second. Since it should relatively a rare event we can live with
the current blocking mode (and there are actually other parts of
b10-auth that have the same problem). But please leave a TODO
comment on this point. We should eventually solve it.
- CounterImpl::sendStats(): the "catch" case doesn't seem to be
covered by the tests
- Counter::getCallback(): this could/should be a const member function.
--
Ticket URL: <http://bind10.isc.org/ticket/347#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list