BIND 10 #2042: Prototype backend performance microbenchmark: MySQL

[BIND 10 Development]

#2042: Prototype backend performance microbenchmark: MySQL
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  tomek
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:  Sprint-
                   Priority:         |  DHCP-20120903
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  dhcpdb                             |           Sub-Project:  DHCP
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 I have reviewed commit 272d9f6f5bb929d81f1236ad953378d145225ade.

 '''General'''
 Minor: In the code you're using mixture of std::cout and printf. Im my
 opinion it would be "cleaner" to use one of them all the time. In any
 case, this is not very important.

 '''tests/tools/dhcp-ubench/dhcp-perf-guide.html'''
 The information in the document is very useful (especially the results
 presented on the graph).
 One concern I usually have with benchmarks is how consistent the results
 are. In other words, if I run this test a couple of times in turn, would
 it give the same result?

 '''tests/tools/dhcp-ubench/benchmark.h'''
 Suggest that void print_clock(...) is ranamed to printClock(...) so as it
 is consistent with naming convention used for other functions. The same
 applies to struct timespec get_time().

 In doxygen comments to several functions there is a reference to Num_
 member that does not exist. It should be replaced with num_ which is
 declared as uint32_t num_ in protected scope.

 '''tests/tools/dhcp-ubench/benchmark.cc'''
 Although it is not a big deal when calling it once, but the following
 assignment hitratio_ = 0.9f in constructor's body could be moved to
 constructor's initialization list.

 parseCmdline(): Error string ''"Failed to iterations (-n option)."'' does
 not make sense to me.

 parseCmdLine(): The lexical_cast<int> prevents specifying non-numeric
 arguments but it will not prevent negative arguments like -n-20. Suggest
 additional check that num_ > 0.

 usage(): Parameter -c is not documented.

 get_time(): clock_gettime may return negative value which indicates error.
 This should be checked prior to returning ts. If it occurs it is fatal
 error.

 get_time(): This is a side note. I have been using
 boost::posix_time::ptime class which I found very useful and more portable
 than clock_gettime. With the latter you have to take care of ifdefs for
 particular OSes.

 '''tests/tools/dhcp-ubench/memfile_ubench.cc'''
 The class members Filename_, Sync_ and File_ should have names starting
 with lowercase letter.

 createLease4Test(): Not very important here but it is recommended to use
 ++i in for() loops rather than i++. The same thing can be found in
 multiple other places too.

 createLease4Test(): For people who are less familiar with the code it
 would be good to have comments what the magic numbers: 19, 65, 33 are. The
 other solution would be to define constants with meaningful names.

 searchLease4Test(): The variable hitRatio should be renamed to hit_ratio.
 Otherwise it suggests that hitRatio is a function.

 '''tests/tools/dhcp-ubench/mysql_ubench.h'''
 MySQL_uBenchmark constructor: descirption of parameter hostname says "Name
 of the hostname to connect to". Suggest that this is changed to "Name of
 the host to connect to".

 '''tests/tools/dhcp-ubench/mysql_ubench.cc'''
 createLease4Test(): Comment at pool_id declaration does not make sense to
 me. Variable is initialized to 1000 while comments says 'Let's use pools
 0-99'

 createLease4Test(): Again, it would be good to have some more comments
 describing magic numbers: 19,65 etc.

 '''tests/tools/dhcp-ubench/sqlite_ubench.cc'''
 disconnect(): Function closes connection if db_ object is does nothing if
 db_ object does not exist. It could be better maybe to throw exception
 (say InvalidOperation) if db_ does not exist to inform caller that nothing
 has been done?> The mysql_ubench::disconnect() behaves that way.

 search_callback(): Shouldn't be the code under #ifdef 0 executed in
 verbose mode and not executed in non-verbose?

-- 
Ticket URL: <http://bind10.isc.org/ticket/2042#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development