BIND 10 #2140: Create abstract API for lease database access

[BIND 10 Development]

#2140: Create abstract API for lease database access
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121004
  medium                             |            Resolution:
                  Component:  dhcp   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DHCP
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => tomek


Comment:

 Reviewed commit 0b7e9646307a6e58d3e1df60c68cbac5dbcbfecb

 '''src/lib/dhcp/duid.cc'''[[BR]]
 DUID constructor: use "const uint8_t* data" instead of "const uint8_t *
 data"

 !ClientId constructor:  use "const uint8_t* clientid" instead of "const
 uint8_t *clientid"


 '''src/lib/dhcp/duid.h'''[[BR]]
 Avoid spaces in definition of operation methods (e.g. "operator==("
 instead of "operator == (")

 getClientId() returns a copy of the client ID, not a reference to it (as
 indicated in the "@brief" description.)

 operator!=(): could probably write this inline - "return (!
 operator==(other))".

 ClientId::getAddress(): (return statement) should not be spaces after an
 opening parenthesis or before closing one.

 Any reason not to split !ClientId and DUID classes (and their tests) into
 separate files (makes it easier to find the code)?

 '''src/lib/dhcp/lease_mgr.h'''[[BR]]
 lease4.fqdn_fwd_ header - we would update A records for an IPv4 lease.

 lease4.options_ header - typo - "databased".

 Is the overhead of keeping a shared point to a client ID justified?  Why
 not keep a copy of the ID?

 getVersion() is a good idea, but we also have the version of the
 underlying database product to consider.  However, I suggest we keep it
 simple for now and just check the schema version. (Anyway, I would expect
 the version of the underlying API (and backend) will be compatible with
 the rest of the code as they will all be distributed together.)  As for
 schema version, I suggest we go for major/minor number (as per BIND) - see
 [ticket:963#comment:2 this comment] on #963 for some details.

 getLeaseX() functions: how often do we expect the lease to be accessed by
 address/hardware address/client ID?  I ask because that affects the
 indexing - we should probably have an index for each of them, but that
 will slow down the rate at which leases can be added to the database
 (because of the need to update indexes).

 '''src/lib/dhcp/tests/duid_unittest.cc'''[[BR]]
 When doing tests, is there a need to create DUIDs though "new"?  Creating
 them on the stack would be equally effective (and would mean that no
 scoped_ptr is needed to guarantee their removal).

 '''src/lib/dhcp/tests/lease_mgr_unittest.cc'''[[BR]]
 Avoid using constructs opf the form "type * variable" - prefer "type*
 variable" (in "constructor" test).

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