BIND 10 #2342: MySQL lease database access functions

[BIND 10 Development]

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

Comment (by stephen):

 Split the modifications into three parts to address the three parts of the
 review.

 Corrections to comment:5 addressed in commit
 b0ed08d21421ca321502cda1e9811a4bfe560275

 '''configure.ac'''
 > Error message "Specified mysql_config program ${MYSQL_CONFIG} is a
 directory" ...
 Combined the two checks into one and modified the error message.

 > Is the mysql_config program called differently on different systems?
 Pehaps specifying a directory is sufficient? ...
 The --with-botan-config switch points to a configuration program, and I
 followed that example.

 > Please write it clearly somewhere that MySQL is currently used for DHCP
 only. Users can easily get an impression that DNS part of BIND10 may use
 MySQL as well.
 The switch has been renamed --with-dhcp-mysql and the help text tries to
 make the "DHCP only" usage clear.

 To avoid confusion, I've also removed the MySQL listing from the final
 report.

 > Why did you change order of the files in lib10_dhcpla_SOURCES? If your
 intention was to make it alphabetically ordered, then iface_mgr_bsd.cc is
 in a wrong place. I'm not opposing that change, just curious about it.
 I use vim, so when I sort a list like that I just run it through the
 external "sort" program.  Unfortunately although LC_ALL was set in my
 .bashrc, it wasn't exported - so sort ignored it and treated "." and "_"
 as equivalent.  This has been corrected.

 '''dhcpdb_create.mysql'''
 > Is the type VARBINARY portable?...We may keep it, but let's add a
 comments section at the end of the schema file about likely portability
 issues.
 Possibly not, but the field is binary, "VARBINARY" seemed proper.  From
 what I understand, this means that sorting and comparison is based on byte
 values instead of a character set.  If we put an index on the column, then
 I guess VARBINARY will offer slightly better performance (no collation
 table to check).  I've noted this down, but it should be trivial to change
 if we ever need to.

 > Length of the client-id field in lease4 table....but I assume that DUID
 replaced or is sent as client-id, so from client identification
 perspective that sets the upper limit at 128 bytes.
 We have to specify something, and 128 characters seems a suitable maximum.
 We can change this if we need.  I've moved the definition of the sizes of
 the various buffers to the head of the file and added some documentation
 (to both the .cc file and the schema definition file) to make it clear
 where changes are needed if the lengths are altered.

 > lease_type TINYINT - you can add a comment "see lease6_types for
 possible values".
 Done.

 > Comment "IA ID" for a field iaid does not give any information..."See
 section 10 of RFC3315" is another possible comment here.
 Changed.

 > Why do you use COMMIT at the end of the script? My understanding is that
 COMMIT finalizes transaction that is in progress, but there is no START
 TRANSACTION or BEGIN anywhere in the script.
 My omission - corrected.

 > Also, it seems valid, but a bit odd to require doing everything in one
 transaction here.
 Changed.

 > Comment for comment about indexes. I thought the most likely index to
 appear first will be for addresses.
 By default, the first field in the table is the primary key and is
 implictly indexed.  I've made this clear by adding the PRIMARY KEY
 description to that field.  :wNote that indexing on a text field -
 currently used to store an IPv6 address - may be slow.  When completed,
 the address will be stored in a BINARY(16) field, and access will be much
 faster.

 '''lease_mgr.h'''
 > Please do not use defines with two leading underscores. These are
 reserved for compiler. LEASE_MGR_H will just fine instead.
 Unfortunately that is used elsewhere in BIND 10 - see the .h files in
 other directories.

 > Lease6() constructor: That is wrong. It passes 0 to IOAddress(uint32_t
 v4address), which in turn creates address 0.0.0.0. That is very wrong.
 This is not really an issue.  The problem is that in getLeaseData(), a
 Lease6 object has to be created and filled in, and it was the lack of a
 default constructor for the addr_ object that was preventing this.  The
 addr_ object is replaced.  However, I've changed the constructor to make
 it a V6 address by initialising with the string "::".

 > As for both Lease4 and Lease6 constructors, I think there should be one,
 but let's not make it default...
 It depends what the easiest thing to do is.  There are a lot of fields and
 a constructor in which all those fields have to be set is cumbersome to
 use.  However, if we need one, I don't mind.  The only place where one is
 used is the MySQL code is in MySqlLease6Exchange::getLeaseData() (so far),
 so adding the constructor is not really an issue.  How many other places
 in the code will it be used?

 > What's your opinion on keeping T1 and T2 in the lease structure? It is
 technically a property of the IA container in v6 or DHCP exchange in v4
 and not part of the lease.
 I think that the lease tables should hold lease data only.  Therefore,
 T1/T2 should probably be removed from the lease structure.

 > !LeaseMgr() constructor. Defining extra type (!ParameterMap) makes the
 interface less robust and actually requires more code in every place where
 !LeaseMgr...
 (Comment ignored - overridden by a later comment.)

 > Thanks for adding the consts and references. I'm not sure how that
 slipped through previous reviews :)
 Nor am I.

 > There is a new method commit(). I'm fine with it, but isn't there
 another one needed like beginTransaction()?
 There is no mysql_start_transaction() call, only mysql_commit() and
 mysql_rollback().  And the MySQL documentation is annoyingly obtuse on
 this point.  From various sources it ''appears'' that with autocommit
 disabled, a new transaction automatically starts with the first statement
 after the last commit.  So it appears that we don't need one.

 > I noticed that there is no page in Developer's Guide about the backend.
 There should be one that explains the core concepts. See
 src/bin/dhcp6/dhcp6.dox for an example. It doesn't have to be very
 thorough. It is addressed at developer's who want to understand the
 architecture. Existing method documentation is fine regarding the details.
 #2403 created for that.

 > !LeaseMgrPtr - I very strongly oppose to it. !LeaseMgr should be a
 singleton.
 Fair point.  The !LeaseMgr creation code is now split into two:
 LeaseMgrFactory::create(dbconfig) to create the lease manager for the
 specified backend, and LeaseMgrFactory::instance() to return the current
 !LeaseMgr.  A destroy() method is also present to clean up - it should be
 called at program end.

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