BIND 10 #2342: MySQL IPv6 lease database access functions

[BIND 10 Development]

#2342: MySQL IPv6 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):

 Corrections to comment:6 addressed in commit
 08daa03beac5d16305734fc434edcc28962205e9

 '''dhcpdb_create.mysql'''
 > The field client_id should be called duid.
 Changed.

 > lease_time should be called valid_lifetime
 Changed.

 '''lease_mgr.cc'''
 > No specific comments, except that !ParameterMap is slightly more
 difficult to use, compared to simple string.
 (Ignored, see below)

 > Also, do we need all those 11 headers if most of the code was removed?
 What headers?


 '''lease_mgr_factory.cc'''
 > Oh, I see that the code for parsing dbconfig string was just moved.
 Please ignore my previous comments about its disappearance from !LeaseMgr
 then.
 Will do.

 > I don't like the idea of shared_ptr to !LeaseMgr...
 See above regarding the splitting of !LeaseMgr creation and access (all
 now in !LeaseMgrFactory).

 > Please add a comment that we will eventually need to develop some kind
 of registration mechanism for different backends
 Added to lease_mgr_factory.h (which seemed the natural place for the
 comment).

 '''mysql_lease_mgr.h'''
 > Please do not use two leading underscores in MYSQL_LEASE_MGR_H define.
 See above regarding what appears to be the norm in BIND 10.

 > !/// @brief Abstract Lease Manager
 > This class is not abstract, at least I really hope it isn't. I plan to
 use it later this week :)
 Too much cutting and ppasting and not enough attention.  Fixed.

 > Do we want to have default values for the host,db name,user and
 password?
 I've set a default for "host" ("localhost"), but I'd like to leave the
 rest.  "Kea" is our current code name and it is entirely possible that it
 may be changed before release.  Default values for "user" and "password"
 tend to be discouraged as people leave the defaults set, opening security
 holes.

 > Comment for !MySqlLeaseMgr() should clarify if the parameters are
 mandatory or optional.
 Done.

 > It should also mention that !DbOperationError or !DbOpenError may be
 thrown.
 Done.


 > Several Lease6 methods do not have possible exceptions mentioned in
 their comments. They should be added.
 Added for methods implemented so far.  Headers will be updated as more
 methods are implemented.

 > Lease4 methods don't have them either, but I think we can skip it for
 now as there is no implementation. We may consider adding @todos marking
 v4 code as not implemented, but I'm not sure if there's much benefit to do
 it as those methods will likely to appear in the upcoming weeks or even
 days.
 I've changed this ticket description to talk about method for V6
 operations.  Another ticket (#2404) has been created for V4.


 > Why many methods are virtual here? Do we expect and further
 specialization, like MySQL55LeaseMgr? Those method are virtual anyway,
 since their definitions in base class are virtual. On a second thought, I
 think we can keep the virtual keyword.
 Agreed.  If a method is virtual in the base class, I like to declare it
 virtual in derived classes, for documentation purposes.

 > Sorry for opening that can of worms, but what timezone the timestamps
 are specified in? There was a long fight over ISC-DHCP using UTC. I'm not
 sure how it ended, but it was frowned upon by many people around the
 world. As a person who enjoys living close to Greenwich you may
 underestimate the issue ;-). However it was very frustrating for people
 who wanted to read actual expiration time from lease file. My point here
 is that we should make it possible for people to let them use their local
 timezones. If that is already the case, a brief comment that is all that
 is needed here.
 MySQL stores timestamps in local time (this is documented in the MySQL
 documentation).  I'll make a note to include that in the developer
 documentation.


 > !StatementIndex. I think the GET_VERSION should come on the list first,
 as this is the first call to be ever called...
 I was going to put the list of statements in alphabetical order.

 > I like the checkError method a lot. It will be very useful for debugging
 as all necessary info in provided in the exception message.
 Thanks.

 '''mysql_lease_mgr.cc'''
 > Comment says that "On the INSERT, SELECT and UPDATE statements, an array
 of MYSQL_BIND structures must be built". I think the same is true for
 DELETE as you need to pass something to WHERE clause.
 Comment has been updated to explain that the class only handles the
 MYSQL_BIND structures describing data exchanged with the database.

 > MySqlLease6Exchange::!CreateBindForSend(): Why bind_[7].is_unsigned is
 set in such a strange way? Using true_ would work.
 Missed that one when I created true_ and false_ for the my_bool type.
 Fixed.

 > What does "The MySQL documentation" mean in the !CreateBindForSend()
 comment?
 Nothing - removed.

 > MySqlLease6Exchange::getLeaseData() - The exception that throws "invalid
 lease type returned" should provide more details.
 Added more information to the exception.

 > MySqlLease6Exchange fields: Why the duid storage is called clientid_ and
 clientid_length_ rather than duid_ and duid_length_?
 IIRC that was what is was called in the performance test program.  It has
 been changed to "duid".

 > MySqlLeaseMgr::converToDatabaseTime(): See my comments about forcing
 users to use UTC. Users prefer to store their data in their local time. If
 MySQL allows storing data in local time, we should use it.
 It uses local time and comments have been added to note this.

 MySqlLeaseMgr::openDatabase()
 Does it make sense to continue without database name? There's a comment
 about using defaults for non-specified options. What defaults? Where are
 they specified or what values they have?

 > MySqlLeaseMgr::prepareStatement(): !InvalidParameter should specify
 faulty index in its text.
 Added.

 > MySqlLeaseMgr::!MySqlLeaseMgr(): We should check if mysql_init()
 returned NULL.
 Done - it throws a !DbOpenError exception with a suitable message.

 > Lease4 methods: I would prefer them to throw until they are
 implemented...
 All unimplemented methods (both Lease6 and Lease6) now throw a
 !NotImplemented exception.


 > MySqlLeaseMgr::getName() - should return "mysql";
 As "name" is used to denote the database, getName() should return
 "keatest", which is how it has been implemented. (A "getType()" should
 return "mysql".)  We may want to check

 '''General comments for !MySqlLeaseMgr class:'''
 > Would it be possible to rename raw_statements_ to text_statements_? Raw
 may mean different things in this context...
 Changed.

 > One final question about the mysql_lease_mgr in general: is the code
 thread-safe?
 Not at the moment.  The underlying mysql_xxxx() calls can be thread-safe,
 but you need to use a different database connection for each thread.  If
 we go this way, we may want to relax the idea of a singleton lease manager
 - we could have multiple lease managers, each with a different connection.

 > One nasty question asked during BIND10 is also adequate here: is the
 code signal safe? I don't think we need to care about it now, just asking
 out of curiosity.
 Probably not very signal safe.  There are no handlers I am aware of.

 '''src/lib/dhcp/Makefile.am'''
 > One thing occurred to me: lease_mgr files should part of the
 libb10-dhcpsrv (server specific stuff), not the libb10-dhcp++ library
 (stuff shared by clients, servers, relays, performance tools etc).
 Files moved to libb10-dhcpsrv

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