BIND 10 #2342: MySQL IPv6 lease database access functions

[BIND 10 Development]

#2342: MySQL IPv6 lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  marcin
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121115
  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 stephen):

 * owner:  stephen => marcin


Comment:

 '''database_backends.dox'''[[BR]]
 > Typo here: A lease manager is instantiated through a the
 !LeaseMgrFactory....
 >
 > Typo here: ... is is controlled by the argument.
 >
 > Typo here: ...there are certain pre-requisites for scuccessfully running
 the unit tests.
 Done.

 > Awkward formatting when enumerating pre-requisites for setting up
 different (currently MySQL only) databases. In particular, These are
 database-specific...
 Done

 > Consider adding references/links to the MySQL documentation on how to
 create MySQL users and schema...
 I've avoided doing that, partly because anyone installing Kea into a MySQL
 system should know how MySQL works (presumably they had to install MySQL
 to start with) but mainly because the references suggested are MySQL
 version-specific, so will become outdated and could be removed.

 > Also, suggest that whenever you write keatest in a sentence it is taken
 between quotation marks.
 I've chosen to use an emphasis instead of quotation marks (so there is no
 confusion as to whether the quotation marks are part of the string the
 user should enter).  In addition, I've emphasised some other keywords.


 '''lease_mgr.h'''[[BR]]
 > updateLease6: This function is pure and thus it doesn't throw any
 exception so I suggest that @exception tags are removed from the
 documentation...
 Done.

 > updateLease6: Although not mandated in style guidelines, I suggest that
 @exception tags are replaced with @throw tags for two reasons...
 All occurrences of "@exception" in the DHCP library replaced with
 "@throw".  In addition, all exceptions now include the namespace to ensure
 they appear as hyperlinks in the Doxygen documentation.


 '''mysql_lease_mgr.cc'''[[BR]]
 > Minor: Included headers: according to BIND10 coding style, if possible
 we should be first including project headers, then library headers and
 lastly system headers.
 Done.


 > MySqlLease6Exchange doxygen documentation: There is extra new line
 between doxygen description of this class and line starting with Note -
 @note tag used

 > MySqlLease6Exchange constructor documentation: Thanks for putting the
 note why you perform memset on addr6_buffer_ etc. However when I read it
 in the html form I get confused what ''other initializations'' it is
 referring to. I suggest that you clarify this comment so it can be
 understood without a need to read the code.
 Done.

 > getLease6: It is not necessary to do complex cast and usually removing
 "constness" is bad idea too. Can't you just initialize duid_vector by
 value:...
 It is possible, but as you note, this involves a redundant copy.  As we
 have to focus on performance, I'm wary about introducing such penalties:
 not because this one will have a significant impact, but because the
 cumulative effect of all these copies through the code may do. I've kept
 the "const_cast" in, but have added a an explanatory note describing why
 the "const" is being removed (because the C interface to MySQL does not
 use "const").

 > getLease6: In the try-catch statement you catch isc::!BadValue exception
 (which may be emitted by getLeaseData()) and free some allocated resources
 if this exception is caught. However it appears to me that getLeaseData()
 may throw other exceptions too...
 Good point.  I've put the call to mysql_stmt_free_result in the destructor
 of an object that is created whenever
 mysql_stmt_fetch is used.  That way, mysql_stmt_free_result should be
 called however getLease6() or any other method exits.


 '''mysql_lease_mgr.h'''[[BR]]
 > updateLease6 doxygen: invalid parameter name lease4 (instead of lease6).
 Also similar issue in @brief description.
 >
 > deleteLease6 doxygen: same as above
 Fixed.


 '''mysql_lease_mgr_unittest.cc'''[[BR]]
 >!MysqlLeaseMgrTest: suggest that you try-catch around this:
 >
 > LeaseMgrFactory::create(validConnectionString());
 >
 > ...
 Done.  All exception are caught, the message output and the exception re-
 thrown.  As the exception may not derive from an ISC exception (and so may
 not have the "what" message), this seems the way to give most information.

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