BIND 10 #2404: MySQL IPv4 lease database access functions

[BIND 10 Development]

#2404: MySQL IPv4 lease database access functions
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121129
  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):

 '''src/lib/dhcpsrv/mysql_lease_mgr.h''': Major version should be
 increased, not minor as the changes are not backwards-compatible.

 '''deleteLease4/6''' returns boolean value, but updateLease4/6 status
 is reported as exception (or lack of thereof). This should be
 consistent. I think exeptions are they way to go (there may be other
 exeptions as well, e.g. db error), but I'm ok with whatever consistent
 approach you'll propose.

 '''deleteLease4/6''' todo comment: I'm not sure about unifying them.
 I think it would be better from the consistency perspective to keep
 the API clearly label protocol number.

 '''StatementIndex''' enum: Comment for GET_LEASE4_CLIENTID_SUBID should be
 {{{
 "// Get lease4 by Client ID and subnet ID".
 }}}

 '''addLeaseCommon()''': it may not be obvious what the two flavours are.
 Adding simple "(v4 or v6)" will clarify that.

 '''getLeaseCollection()''': "any data in the collection is cleared before
 new data is added.". Why? I think it *may* be useful to obtain sum of
 all leases that meet certain criteria. For example when processing
 leasequery, we may be interested in leases for a given client-id. We
 could query several subnets and just keep adding results to the same
 collection. I'm not sure if that is how we will implement leasequery,
 but I'm somewhat doubtful about result.clear(). If you have strong
 opinion about keeping it as it is now, I won't object.

 I like the templated getLeaseCollection approach. Do you think
 specifying 3 parameter getLeaseCollection methods are needed?
 Templated getLeaseCollection could have been called directly. The only
 (minor) drawback is that you'd need to use exchange4_ and exchange6_
 explicitly in every call. Again, that is only a suggestion. I'm fine
 with keeping the code as it is now.

 '''src/lib/dhcpsrv/tests/schema_copy.h''': I think that keeping this
 file completely up to date with dhcpdb_create.mysql will be a
 challenging task. We will see how it goes. One possible approach would
 be to have the schema defined in one place and the other being
 generated out of it. But let's keep things simple for now. Please just
 add a statement to dhcpdb_create.mysql that any changes to the schema
 require also schema_copy.h update. Please add is somewhere close to
 the schema_version statements.

 '''src/lib/dhcpsrv/tests/lease_mgr_unittest.cc''': Please include a
 one or two line comment before each test to explain the scope of a
 test. That is Shawn's idea that I very much support. We will convert
 them to a more structured test documentation one day.

 '''Lease4.Lease4Constructor''': I would use much bigger values for
 ADDRESS. Currently it is 0.1.some.thing. It would be convenient to
 check that values with most significant bit set (greater than
 0x7fffffff) work as well.

 Please update copyright header to 2012. LeaseMgr is so advanced that
 nobody even dared to think about it in 2011 :)

 '''src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc''':

 You indented "if (name != NULL) {" line. Previous indentation was
 correct. Please revert the change.

 boost::shared_ptr<DUID> is used in many places. There is DuidPtr type
 defined for it. Also, we probably should defined ClientIdPtr (or
 perhaps use DuidPtr for as a ClientId is derived from Duid class).

 '''checkLeasesDifferent()''': I agree with the first loop using
 ASSERT_TRUE
 istead of EXPECT_TRUE, but I question the second double loop: It will
 print
 out the first error and abort. It would be more informational if all
 combinations were printed. Also, it should be EXPECT_EQ or ASSERT_EQ,
 so leases would be printed in case of mismatch. Finally, I think it would
 be good to compare not the shared pointers, but the objects they point to.
 This would catch the case where you have 2 exact copies with all
 parameters
 the same.

 '''getLease4AddressSubnetId''': Please define constants for the values
 used
 in tests (73, 74).

 '''getLease4ClientId, getLease4HwaddrSubnetId, getLease4Hwaddr''': Please
 check that the functions do not crash when empty client-id and/or hw
 address is provided. (Remember the fun we had with zero length
 client-ids?) Please test maximum sizes of the client-id (128) and
 hwaddr (20).

 One generic comment about client-id: Shawn suggested that we should
 keep hardware type as the first byte in hwaddr. That is something we
 are doing in isc-dhcp. I'm not saying that we should do it within this
 ticket, but we should keep in mind that something like this will
 likely be implemented. The best course of action would be to talk about
 if briefly and the probably create a ticket for it.

 Generic comment about getLeaseX family of tests. They should be
 eventually refactored to cover other backends. I think we need one
 common class for all backends. As this is a huge task, we should
 probably create a separate ticket for it. I for one would like to
 avoid replicating this huge work you've done here when implementing
 similar tests for memfile.

 This concludes the code review. Additional comments will follow.

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