Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#3681 closed enhancement (complete)

Common MySQL Connector Pool

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.0-beta
Component: database-all Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

There is already a class that connects to MySQL, MySQLLeaseMgr, which connects to MySQL and operates on leases. We need a class that will connect to MySQL and extract host information.

Instead of copying over the code from MySQLLeaseMgr to MySQLHostDataSource, it would be better to create a common base class used by both MySQLLeaseMgr and MySQLHostDataSource. This class would do all operations that are common - connect to the database, maintain connection, close connection and propably prepare compiled statements. Let's call this new class MySqlConnection?.

After this ticket is complete, the hierarchy should be as follows:

class MySQLConnection {
 /// all generic MySQL code here
};

MySQLLeaseMgr : public LeaseMgr, public MySQLConnection {
 /// lease specific code here
};

MySqlHostDataSource : public BaseHostDataSource, public MySQLConnection {
 /// empty for now, will fill this in as part of #3567
};

Subtickets

Attachments (1)

0001-ticket-3681.patch (58.0 KB) - added by tomek 5 years ago.
Patch as submitted by Adam Kalmus

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0

comment:2 Changed 5 years ago by kalmus

  • Owner set to kalmus
  • Status changed from new to assigned

comment:3 Changed 5 years ago by tomek

  • Milestone changed from Kea1.0 to Kea0.9.1

comment:4 Changed 5 years ago by marcin

  • Milestone changed from Kea0.9.1 to Kea1.1

Changed 5 years ago by tomek

Patch as submitted by Adam Kalmus

comment:5 Changed 5 years ago by tomek

I applied this patch (see trac3681, commit 9a70b722a539c43c1931297d3d6ae66746e42017), but the code does not compile:

$ make -j2
make  all-recursive
make[1]: Entering directory `/home/thomson/devel/kea2/src/lib/dhcpsrv'
Making all in .
make[2]: Entering directory `/home/thomson/devel/kea2/src/lib/dhcpsrv'
  CXX      libkea_dhcpsrv_la-alloc_engine.lo
  CXX      libkea_dhcpsrv_la-lease_mgr_factory.lo
In file included from alloc_engine.cc:19:0:
../../../src/lib/dhcpsrv/lease_mgr_factory.h:109:12: error: 'ParameterMap' in 'class isc::dhcp::LeaseMgr' does not name a type
     static LeaseMgr::ParameterMap parse(const std::string& dbaccess);
            ^
../../../src/lib/dhcpsrv/lease_mgr_factory.h:120:19: error: 'ParameterMap' in 'class isc::dhcp::LeaseMgr' does not name a type
             const LeaseMgr::ParameterMap& parameters);
                   ^
../../../src/lib/dhcpsrv/lease_mgr_factory.h:120:43: error: ISO C++ forbids declaration of 'parameters' with no type [-fpermissive]
             const LeaseMgr::ParameterMap& parameters);
                                           ^
In file included from lease_mgr_factory.cc:18:0:
../../../src/lib/dhcpsrv/lease_mgr_factory.h:109:12: error: 'ParameterMap' in 'class isc::dhcp::LeaseMgr' does not name a type
     static LeaseMgr::ParameterMap parse(const std::string& dbaccess);
            ^
../../../src/lib/dhcpsrv/lease_mgr_factory.h:120:19: error: 'ParameterMap' in 'class isc::dhcp::LeaseMgr' does not name a type
             const LeaseMgr::ParameterMap& parameters);
                   ^
../../../src/lib/dhcpsrv/lease_mgr_factory.h:120:43: error: ISO C++ forbids declaration of 'parameters' with no type [-fpermissive]
             const LeaseMgr::ParameterMap& parameters);
                                           ^
In file included from lease_mgr_factory.cc:21:0:
../../../src/lib/dhcpsrv/mysql_lease_mgr.h:42:7: error: redefinition of 'class isc::dhcp::MySqlHolder'
 class MySqlHolder : public boost::noncopyable {
       ^
In file included from ../../../src/lib/dhcpsrv/memfile_lease_mgr.h:24:0,
                 from lease_mgr_factory.cc:19:
../../../src/lib/dhcpsrv/mysql_connection.h:103:7: error: previous definition of 'class isc::dhcp::MySqlHolder'
 class MySqlHolder : public boost::noncopyable {
       ^
In file included from lease_mgr_factory.cc:21:0:
../../../src/lib/dhcpsrv/mysql_lease_mgr.h:81:16: error: redefinition of 'const uint32_t isc::dhcp::CURRENT_VERSION_VERSION'
 const uint32_t CURRENT_VERSION_VERSION = 2;
                ^
In file included from ../../../src/lib/dhcpsrv/memfile_lease_mgr.h:24:0,
                 from lease_mgr_factory.cc:19:
../../../src/lib/dhcpsrv/mysql_connection.h:142:16: error: 'const uint32_t isc::dhcp::CURRENT_VERSION_VERSION' previously defined here
 const uint32_t CURRENT_VERSION_VERSION = 3;  // version 3: adding host managment features
                ^
In file included from lease_mgr_factory.cc:21:0:
../../../src/lib/dhcpsrv/mysql_lease_mgr.h:82:16: error: redefinition of 'const uint32_t isc::dhcp::CURRENT_VERSION_MINOR'
 const uint32_t CURRENT_VERSION_MINOR = 0;
                ^
In file included from ../../../src/lib/dhcpsrv/memfile_lease_mgr.h:24:0,
                 from lease_mgr_factory.cc:19:
../../../src/lib/dhcpsrv/mysql_connection.h:143:16: error: 'const uint32_t isc::dhcp::CURRENT_VERSION_MINOR' previously defined here
 const uint32_t CURRENT_VERSION_MINOR = 0;
                ^
../../../src/lib/dhcpsrv/mysql_connection.h: In member function 'void isc::dhcp::MySqlLeaseMgr::checkError(int, isc::dhcp::MySqlConnection::StatementIndex, const char*) const':
../../../src/lib/dhcpsrv/mysql_connection.h:285:17: error: 'isc::dhcp::MySqlHolder isc::dhcp::MySqlConnection::mysql_' is private
     MySqlHolder mysql_;
                 ^
In file included from ../../../src/lib/log/logger.h:25:0,
                 from ../../../src/lib/log/macros.h:18,
                 from ../../../src/lib/dhcpsrv/dhcpsrv_log.h:19,
                 from lease_mgr_factory.cc:17:
../../../src/lib/dhcpsrv/mysql_lease_mgr.h:575:35: error: within this context
                       mysql_error(mysql_) << " (error code " <<
                                   ^
../../../src/lib/exceptions/exceptions.h:186:18: note: in definition of macro 'isc_throw'
         oss__ << stream; \
                  ^
In file included from ../../../src/lib/dhcpsrv/memfile_lease_mgr.h:24:0,
                 from lease_mgr_factory.cc:19:
../../../src/lib/dhcpsrv/mysql_connection.h:285:17: error: 'isc::dhcp::MySqlHolder isc::dhcp::MySqlConnection::mysql_' is private
     MySqlHolder mysql_;
                 ^
In file included from ../../../src/lib/log/logger.h:25:0,
                 from ../../../src/lib/log/macros.h:18,
                 from ../../../src/lib/dhcpsrv/dhcpsrv_log.h:19,
                 from lease_mgr_factory.cc:17:
../../../src/lib/dhcpsrv/mysql_lease_mgr.h:576:35: error: within this context
                       mysql_errno(mysql_) << ")");
                                   ^
../../../src/lib/exceptions/exceptions.h:186:18: note: in definition of macro 'isc_throw'
         oss__ << stream; \
                  ^
In file included from lease_mgr_factory.cc:24:0:
../../../src/lib/dhcpsrv/pgsql_lease_mgr.h: At global scope:
../../../src/lib/dhcpsrv/pgsql_lease_mgr.h:149:25: error: 'ParameterMap' does not name a type
     PgSqlLeaseMgr(const ParameterMap& parameters);
                         ^
../../../src/lib/dhcpsrv/pgsql_lease_mgr.h:149:39: error: ISO C++ forbids declaration of 'parameters' with no type [-fpermissive]
     PgSqlLeaseMgr(const ParameterMap& parameters);
                                       ^
lease_mgr_factory.cc:49:1: error: 'ParameterMap' in 'class isc::dhcp::LeaseMgr' does not name a type
 LeaseMgr::ParameterMap
 ^
lease_mgr_factory.cc:79:45: error: 'ParameterMap' in 'class isc::dhcp::LeaseMgr' does not name a type
 LeaseMgrFactory::redactedAccessString(const LeaseMgr::ParameterMap& parameters) {
                                             ^
lease_mgr_factory.cc:79:69: error: ISO C++ forbids declaration of 'parameters' with no type [-fpermissive]
 LeaseMgrFactory::redactedAccessString(const LeaseMgr::ParameterMap& parameters) {
                                                                     ^
lease_mgr_factory.cc: In static member function 'static std::string isc::dhcp::LeaseMgrFactory::redactedAccessString(const int&)':
lease_mgr_factory.cc:83:20: error: 'isc::dhcp::LeaseMgr::ParameterMap' has not been declared
     for (LeaseMgr::ParameterMap::const_iterator i = parameters.begin();
                    ^
lease_mgr_factory.cc:83:49: error: expected ';' before 'i'
     for (LeaseMgr::ParameterMap::const_iterator i = parameters.begin();
                                                 ^
lease_mgr_factory.cc:84:10: error: 'i' was not declared in this scope
          i != parameters.end(); ++i) {
          ^
lease_mgr_factory.cc:84:26: error: request for member 'end' in 'parameters', which is of non-class type 'const int'
          i != parameters.end(); ++i) {
                          ^
lease_mgr_factory.cc: In static member function 'static void isc::dhcp::LeaseMgrFactory::create(const string&)':
lease_mgr_factory.cc:112:5: error: 'ParameterMap' is not a member of 'isc::dhcp::LeaseMgr'
     LeaseMgr::ParameterMap parameters = parse(dbaccess);
     ^
lease_mgr_factory.cc:112:28: error: expected ';' before 'parameters'
     LeaseMgr::ParameterMap parameters = parse(dbaccess);
                            ^
lease_mgr_factory.cc:113:49: error: 'parameters' was not declared in this scope
     std::string redacted = redactedAccessString(parameters);
                                                 ^
lease_mgr_factory.cc:127:60: error: cannot allocate an object of abstract type 'isc::dhcp::MySqlLeaseMgr'
         getLeaseMgrPtr().reset(new MySqlLeaseMgr(parameters));
                                                            ^
In file included from lease_mgr_factory.cc:21:0:
../../../src/lib/dhcpsrv/mysql_lease_mgr.h:97:7: note:   because the following virtual functions are pure within 'isc::dhcp::MySqlLeaseMgr':
 class MySqlLeaseMgr : public LeaseMgr, public MySqlConnection {
       ^
In file included from ../../../src/lib/dhcpsrv/lease_mgr_factory.h:18:0,
                 from lease_mgr_factory.cc:18:
../../../src/lib/dhcpsrv/lease_mgr.h:313:25: note: 	virtual std::string isc::dhcp::LeaseMgr::getType() const
     virtual std::string getType() const = 0;
                         ^
../../../src/lib/dhcpsrv/lease_mgr.h:321:25: note: 	virtual std::string isc::dhcp::LeaseMgr::getName() const
     virtual std::string getName() const = 0;
                         ^
../../../src/lib/dhcpsrv/lease_mgr.h:328:25: note: 	virtual std::string isc::dhcp::LeaseMgr::getDescription() const
     virtual std::string getDescription() const = 0;
                         ^
../../../src/lib/dhcpsrv/lease_mgr.h:344:43: note: 	virtual std::pair<unsigned int, unsigned int> isc::dhcp::LeaseMgr::getVersion() const
     virtual std::pair<uint32_t, uint32_t> getVersion() const = 0;
                                           ^
../../../src/lib/dhcpsrv/lease_mgr.h:350:18: note: 	virtual void isc::dhcp::LeaseMgr::commit()
     virtual void commit() = 0;
                  ^
../../../src/lib/dhcpsrv/lease_mgr.h:356:18: note: 	virtual void isc::dhcp::LeaseMgr::rollback()
     virtual void rollback() = 0;
                  ^
make[2]: *** [libkea_dhcpsrv_la-lease_mgr_factory.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [libkea_dhcpsrv_la-alloc_engine.lo] Error 1
make[2]: Leaving directory `/home/thomson/devel/kea2/src/lib/dhcpsrv'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/thomson/devel/kea2/src/lib/dhcpsrv'
make: *** [all] Error 2

I did refactor the changes significantly. Added new common ancestor (DataSource?), as Adam proposed. Moved around various parts of the code. Updated Makefiles, so the new files are being compiled.
Added new unit-test for the new class.

The code and unit-tests now compile ok. I checked that with both MySQL and Postgres enabled.

Most of the tests are passing, but some of them are failing. They must be fixed.

There's still a bit of work to do here. All parameters must be documented in the header files, some methods may be moved around, e.g. LeaseMgr::parse() and possibly LeaseMgr::redactedAccessString() may probably be moved to DataSource?.

It may be useful to review all the changes done in this branch and decide whether they are really necessary. Use git diff master...trac3681 for that.

When changing a file, please update copyright years (the first line in every file).

Please do not use Polish comments. While Polish is very cool language, there are Kea developers who haven't learned Polish yet :)

ChangeLog? entry is needed.

As this is still work in progress, I'm moving this to assigned state.

comment:6 Changed 5 years ago by kalmus

I've sent patch with some changes, as requested above:

  • Methods parse() and redactedAccessString() are now moved to DataSource? class
  • Applied some changes to tests, so now they all pass, added changes from ticket #3567 before running them.
  • Added some comments, updated copyright years

Proposed ChangeLog entry:

XXX.	[func]		kalmus
	 Created MySqlConnection and DataSource classes to manage common database 
         operations. Redistributed some methods from LeaseMgr and MySqlLeaseMgr classes.
         Updated tests.

	(Trac #3681, git xxx)

And sorry for those polish comments, they are probably some leftovers that I forgot to remove :)

comment:7 Changed 5 years ago by kalmus

  • Owner changed from kalmus to tomek
  • Status changed from assigned to reviewing

comment:8 Changed 4 years ago by tomek

Finally found time to review this patch. Sorry it took so long. I commited your patch as is and then added several fixeds.

After applying this patch to trac3681, commit
7b42484c185b5a395052ac11d8f049b0c2701829, configure fails with the
following error:

configure.ac:1390: error: required file 'src/bin/admin/scripts/mysql/upgrade_2.0_to_3.0.sh.in' not found
autoreconf: automake failed with exit status: 1

Fortunately, that's easy to fix. Copied over missing file from master.

src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
Interesting comment about pgsql test failing if auth-method is set to
trust.

Copyright years should be separated by dashes if the changes were done
in consetutive years (2014-2015) or by comas, if years are not
consecutive (2012,2015). Fixed that.

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc
This file has copyright year updated, but there are no changes here.
I removed this change.

src/lib/dhcpsrv/tests/lease_mgr_factory_unittest.cc
This file has several tabs. I converted them to spaces.

Since all tests are now testing DataSource? methods, they should be
moved to the data_source_unittest.cc.

src/lib/dhcpsrv/lease_mgr_factory.h
Copyright year should be 2012-2013,2015 (Stephen forgot to update it
when he made a change in 2013). I've fixed that.

src/lib/dhcpsrv/data_source.h
Class comment contained tabs. Converted to spaces.

src/lib/dhcpsrv/data_source.cc
Why does this file include both mysql and pgsql leases? What if there
is only one of them installed? The header inclusion will cause an
error about missing mysql.h or libpq-fe.h.

Fortunately, those headers were not required, so I removed them.

Also, there was missing require #include <vector> header. Added it.

/src/lib/dhcpsrv/memfile_lease_mgr.cc|h
This class is derived from MySqlConnection?. It's a mistake. It should
be from DataSource?. Fixed that as well.

AUTHORS
There is a trailing space. Removed.


Ok, this seems to complete the review. Now I will rebase this branch to master and then will push it to our Jenkins test farm. If those tests will pass, the code will be merged.

comment:9 Changed 4 years ago by tomek

I also fixed one extra error: mysql_connection.h was included in lease_mgr.cc and memfile_lease_mgr.cc.

I rebased the code to that latest master and pushed it to trac3681_rebase. Will continue working on this tomorrow.

comment:10 follow-ups: Changed 4 years ago by tomek

After rebase, the following unit-tests fail:

[ RUN      ] MySqlOpenTest.OpenDatabase
mysql_lease_mgr_unittest.cc:138: Failure
Value of: mysql_query(mysql, create_statement[i])
  Actual: 1
Expected: 0
Failed on statement 19: CREATE TRIGGER host_BDEL BEFORE DELETE ON hosts FOR EACH ROW BEGIN DELETE FROM ipv6_reservations WHERE ipv6_reservations.host_id = OLD.host_id; END 
[  FAILED  ] MySqlOpenTest.OpenDatabase (1579 ms)

in src/lib/dhcpsrv.

And this one:

START TEST mysql.lease-init
Wiping whole database keatest
Checking if there is a database initialized already. Please ignore errors.
Initializing database using script /home/thomson/devel/kea2/src/bin/admin/scripts/mysql/dhcpdb_create.mysql
ERROR 1050 (42S01) at line 43: Table 'lease4' already exists
mysql returned status code 1
Assertion failure: 0 != 1, for val1=0, val2=1
kea-admin lease-init mysql returned non-zero status code 0, expected 1
FAILED mysql.lease-init

make[3]: *** [check-local] Błąd 1

in src/bin/admin.

comment:11 in reply to: ↑ 10 Changed 4 years ago by tomek

Replying to tomek:

After rebase, the following unit-tests fail:

[ RUN      ] MySqlOpenTest.OpenDatabase
mysql_lease_mgr_unittest.cc:138: Failure
Value of: mysql_query(mysql, create_statement[i])
  Actual: 1
Expected: 0
Failed on statement 19: CREATE TRIGGER host_BDEL BEFORE DELETE ON hosts FOR EACH ROW BEGIN DELETE FROM ipv6_reservations WHERE ipv6_reservations.host_id = OLD.host_id; END 
[  FAILED  ] MySqlOpenTest.OpenDatabase (1579 ms)

in src/lib/dhcpsrv.

This one is now fixed. The issue was in the order of tables in destroy_statement in schema_mysql_copy.h. It's not possible to delete hosts before ipv6_reservations as the latter has constraints on the former.

comment:12 in reply to: ↑ 10 Changed 4 years ago by tomek

Replying to tomek:

And this one:

START TEST mysql.lease-init
Wiping whole database keatest
Checking if there is a database initialized already. Please ignore errors.
Initializing database using script /home/thomson/devel/kea2/src/bin/admin/scripts/mysql/dhcpdb_create.mysql
ERROR 1050 (42S01) at line 43: Table 'lease4' already exists
mysql returned status code 1
Assertion failure: 0 != 1, for val1=0, val2=1
kea-admin lease-init mysql returned non-zero status code 0, expected 1
FAILED mysql.lease-init

make[3]: *** [check-local] Błąd 1

in src/bin/admin.

Failure reason for this one was similar. mysql_wipe() function from mysql_tests.sh didn't work, as it tried
to delete hosts before ipv6_reservations. That failed and tables were left in the db. The next attempt
to recreate the schema failed.

Fix has been pushed to trac3681_rebase.

Ok, the code seems to be working on my box. Let's see if it will work on our Jenkins farm.

comment:13 Changed 4 years ago by tomek

The code is on trac3681_rebase branch. Running tests on jenkins:
FreeBSD 10.1 (no databases): https://jenkins.isc.org/view/Kea_BuildFarm-developers/job/FreeBSD10.1_64_branch/13/
Ubuntu (pgsql): https://jenkins.isc.org/view/Kea_BuildFarm-developers/job/Ubuntu_14.04_32_pgsql_branch/5/
Ubuntu (mysql): https://jenkins.isc.org/view/Kea_BuildFarm-developers/job/Ubuntu_14.04_64_mysql_branch/3/

Those links above may require Jenkins login to be viewed.

The last one seem to have failed one unit-test MySqlOpenTest?.OpenDatabase?. This test passed on my Ubuntu (with MySQL enabled).

Last edited 4 years ago by tomek (previous) (diff)

comment:14 follow-up: Changed 4 years ago by tomek

  • Owner changed from tomek to marcin

This patch was submitted by Adam and I reviewed it. It matches the general host reservation design. However, I changed couple things, so it would be good if another engineer could take a quick look at those changes. Marcin graciously agreed to do it. Thanks!

comment:15 in reply to: ↑ 14 Changed 4 years ago by marcin

Replying to tomek:

This patch was submitted by Adam and I reviewed it. It matches the general host reservation design. However, I changed couple things, so it would be good if another engineer could take a quick look at those changes. Marcin graciously agreed to do it. Thanks!

Sorry for not getting round to this yet. I will try to have a look tomorrow morning.

comment:16 Changed 4 years ago by kalmus

Hi, Marcin, I took a quick look at the failing tests yesterday and I'm guessing that the mysql_wipe() method still isn't working properly. Right after using mysql_wipe() basic SELECT query returns no errors, as it should if the wipe was performed correctly... Maybe this will be a good point to start.

comment:17 follow-ups: Changed 4 years ago by marcin

  • Owner changed from marcin to tomek

I reviewed the trac3681 branch. This was rather a quick look at the code. A couple of comments.

src/lib/dhcpsrv/data_source.cc
getParameter: the exception should include the name of the parameter which hasn't been found.

src/lib/dhcpsrv/data_source.h
As a high level comment I'd say that this class has a confusing name. The DataSource appears to be a base class for the MySqlConnection, and it merely parses database access parameters. It is not really a source of data like, for example, HostDataSource which retrieves the data. So, I suggest renaming this class to something like BaseDatabaseConnection to better reflect the purpose of this class.

I also see some defficiency in the description of this class. Since, it is going to be a base class for other classes it would be useful to state it. For example, the description says that this class provides common functions for establishing connections. Please say what kind of common functions, e.g. "functions for parsing the parameters in the form of keyword=value". Also, this class doesn't really establish any connection, or even part of it. In other words, some more precision in this text would be desired.

A nit: the declaration of the redactedAccessString has weird indentation. And the DataSource in the DataSource::ParameterMap? can probably be omitted.

Some brief descriptions in this header should start with capital letter.

src/lib/dhcpsrv/mysql_connection.cc
I don't understand why you need explicit "this" in the prepareStatement function, e.g.

    // All OK, so prepare the statement
    this->text_statements_[index] = std::string(text);
    this->statements_[index] = mysql_stmt_init(mysql_);
    if (this->statements_[index] == NULL) {
        isc_throw(DbOperationError, "unable to allocate MySQL prepared "
                  "statement structure, reason: " << mysql_error(mysql_));
    }

src/lib/dhcpsrv/mysql_connection.h
All members of the MySqlConnection class must have descriptions. Also, I don't understand why statements and text_statements_ are public.

From the overall structure, I gather that the MySqlLeaseMgr should not derive from the MySqlConnection. The public inheritance is the "is a" relationship which means that the MySqlLeaseMgr is the MySqlConnection. In english, the "manager" is the "connection" which doesn't sound quite right. The lease manager should rather use the connection, which is that the connection should be a member of the manager (composition). Note that besides the oddity of the hierarchy where the manager is a connection, there is a thing about (perhaps future) possibility for the manager to hold multiple SQL connections. Why not? One would backup another connection. This would also prevent multi-inheritance in the managers.

prepareStatements: neither of the parameters is described.

comment:18 in reply to: ↑ 17 Changed 4 years ago by fdupont

Replying to marcin:

So, I suggest renaming this class to something like BaseDatabaseConnection to better reflect the purpose of this class.

=> the convention is more to put the !Base at the end (i.e., DatabaseConnectionBase). Note if there is no DatabaseConnection IMHO one can remove the !Base. Of course this means the documentation should say the class is a base class (it should anyway :-). If it is abstract (virtual method = 0) this should be in the documentation too.

comment:19 in reply to: ↑ 17 Changed 4 years ago by tomek

  • Owner changed from tomek to marcin

All changes committed to trac3681_rebase.

Replying to marcin:

I reviewed the trac3681 branch. This was rather a quick look at the code. A couple of comments.

src/lib/dhcpsrv/data_source.cc
getParameter: the exception should include the name of the parameter which hasn't been found.

It prints the name now.

src/lib/dhcpsrv/data_source.h
As a high level comment I'd say that this class has a confusing name. The DataSource appears to be a base class for the MySqlConnection, and it merely parses database access parameters. It is not really a source of data like, for example, HostDataSource which retrieves the data. So, I suggest renaming this class to something like BaseDatabaseConnection to better reflect the purpose of this class.

Renamed to DatabaseConnection?. I chose to not use Base in its name, because the name is already long and with extra Base would be too long. There is no other similar class, so I hope it's ok to skip it. The class documentation was extended and is now very clear that the class is a base class for database connection representation.

I also see some defficiency in the description of this class. Since, it is going to be a base class for other classes it would be useful to state it. For example, the description says that this class provides common functions for establishing connections. Please say what kind of common functions, e.g. "functions for parsing the parameters in the form of keyword=value". Also, this class doesn't really establish any connection, or even part of it. In other words, some more precision in this text would be desired.

Description updated.

A nit: the declaration of the redactedAccessString has weird indentation. And the DataSource in the DataSource::ParameterMap? can probably be omitted.

Updated.

Some brief descriptions in this header should start with capital letter.

Some brief descriptions start with a capital letter.

src/lib/dhcpsrv/mysql_connection.cc
I don't understand why you need explicit "this" in the prepareStatement function, e.g.

    // All OK, so prepare the statement
    this->text_statements_[index] = std::string(text);
    this->statements_[index] = mysql_stmt_init(mysql_);
    if (this->statements_[index] == NULL) {
        isc_throw(DbOperationError, "unable to allocate MySQL prepared "
                  "statement structure, reason: " << mysql_error(mysql_));
    }

It was unnecessary. Removed.

src/lib/dhcpsrv/mysql_connection.h
All members of the MySqlConnection class must have descriptions. Also, I don't understand why statements and text_statements_ are public.

They are public, because they're used extensively in classes that use instances of MySqlConnection?. In particular, statements is first set and later used by MySqlLeaseMgr?. The same will be true for upcoming MySqlHostDataSource?.


From the overall structure, I gather that the MySqlLeaseMgr should not derive from the MySqlConnection. The public inheritance is the "is a" relationship which means that the MySqlLeaseMgr is the MySqlConnection. In english, the "manager" is the "connection" which doesn't sound quite right. The lease manager should rather use the connection, which is that the connection should be a member of the manager (composition). Note that besides the oddity of the hierarchy where the manager is a connection, there is a thing about (perhaps future) possibility for the manager to hold multiple SQL connections. Why not? One would backup another connection. This would also prevent multi-inheritance in the managers.

Updated as suggested. MySqlLeaseMgr? is now derived from LeaseMgr? (single inheritance) and MySqlConnection? is a member. As a result, access patterns to MySqlConnection? implicated public access to several members in MySqlConnection?.

prepareStatements: neither of the parameters is described.

They are described now.


Ok, that seems to address all your comments. If you don't have any new issues to raise, I think this code is more or less ready to go. Hopefully, another rebase to current master won't be too problematic.

comment:20 Changed 4 years ago by marcin

  • Owner changed from marcin to tomek

I have reviewed commit 278a4e3117213c0af6ececbef24471b67d39ebf7

and applied a number of small changes, mostly concerning documentation and headers inclusion. Please pull.

The following issues I left unfixed.

src/lib/dhcpsrv/memfile_lease_mgr.h
The inheritance of the Memfile_LeaseMgr should be also modified to not derive from the DatabaseConnection.

src/lib/dhcpsrv/tests/data_source_unittest.cc
This should be renamed to database_connection_unittest.cc

comment:21 Changed 4 years ago by tomek

  • Resolution set to complete
  • Status changed from reviewing to closed

Adam sent me an updated patch. I cleaned it up a bit: instead of reimplementing DatabaseConnection? methods in MemfileLeaseMgr?, I instantiated DatabaseConnection? object in MemfileLeaseMgr?.

This code has passed unit-tests on my system Ubuntu 14.04 and on Jenkins as well.

Adam mentioned that MySQLLeaseMgrTest.OpenDatabase? fails sporadically, but I was not able to reproduce it. If the problem manifests itself, we'll investigate.

Code merged. Thanks a lot for your work on this. Closing ticket.

Phew, that was a long ticket.

comment:22 Changed 4 years ago by stephen

  • Milestone changed from Kea1.1 to Kea1.0

comment:23 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.