BIND 10 #2342: MySQL IPv6 lease database access functions

[BIND 10 Development]

#2342: MySQL IPv6 lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  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 tomek):

 Replying to [comment:8 stephen]:
 > The --with-botan-config switch points to a configuration program, and I
 followed that example.
 Ok.

 > > 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? Printing out MySQL parameters is very useful, we just need to make
 them as DHCP-only. It makes sense to also label existing SQLite as DNS-
 only. Perhaps existing "Features:" could be renamed in "DNS features:" and
 the MySQL section your removed could be named "DHCP Features:".

 > 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.
 I didn't know that you can sort such things automatically. My emacs-fu
 seems weak today :)

 > '''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.
 Our customer asks to do two conflicting things at the same time: keep
 portability and be as efficient as possible (with implies using MySQL
 specific things). Since we decided to focus on performance, I think with
 the portability we just need to document what we think will be the issues
 to solve once someone asks us to run Oracle or PostgreSQL. A simple list
 of potential issues will do just fine. Otherwise we could spend months on
 finding super portable solutions without even what we are trying port to.

 > '''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.
 Mistakes made elsewhere does not warrant creating new mistakes. Double
 underscore defines are reserved for compiler's internal use and no coding
 guidelines or existing practices can overrule that. This question may be
 useful:
 http://stackoverflow.com/questions/224397/why-do-people-use-double-
 underscore-so-much-in-c

 > > 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?
 There is another one in allocation engine. I think that adding a
 constructor will be easier to add new fields later (remember that we still
 need to extend the lease structures with DDNS and failover stuff). It will
 be easy to spot places where the structure is initialized once you add new
 parameter. However, having said that I've added a constructor in #2324, so
 you don't need to do it.

 > > 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.
 Ok. Let's do that once we merge #2324 and #2342.

 > > 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.
 Remember that this interface is generic, so we will likely have to take a
 look at how other backends do it.

 > > 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.
 Ok.

 > > !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.
 Ok, great. Another two possible cases when we want to destroy leasemgr is
 when we are changing backends (not likely to happen in 2012 or 2013) and
 also when running several tests (happened already for allocation engine).

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