Opened 3 years ago

Closed 3 years ago

#5096 closed enhancement (complete)

DhcpX: migrate database configuration

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea 1.2 - Mozilla Milestone 1
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets: #5019, #5037
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1
Total Hours: 1 Internal?: no

Description (last modified by tomek)

Need to migrate database configuration to SimpleParser.

Subtickets

Change History (16)

comment:1 Changed 3 years ago by fdupont

  • Summary changed from DhcpvX: migrate database configuration to DhcpX: migrate database configuration

comment:2 Changed 3 years ago by tomek

  • Parent Tickets set to 5019, 5037

comment:3 Changed 3 years ago by fdupont

This ticket depends on #5035 because of the "type" bison rule split.

comment:4 Changed 3 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:5 Changed 3 years ago by fdupont

Wait for trac5044/trac5044fd (#5044).

comment:6 Changed 3 years ago by fdupont

Resuming!

comment:7 Changed 3 years ago by fdupont

Did the flex/bison part.

comment:8 Changed 3 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Done. BTW if the parser order still matters the lease database part should be moved (I left a comment about this).
Ready for review.

comment:9 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 2
  • Description modified (diff)
  • Owner changed from UnAssigned to fdupont
  • Total Hours changed from 0 to 2

comment:10 follow-up: Changed 3 years ago by tomek

  • Description modified (diff)

Oops, I modified the ticket incorrectly. Fixing it now.

dbaccess_parser.cc
I have some doubts about this code:

    if ((dbtype != "memfile") &&
        (dbtype != "mysql") &&
        (dbtype != "postgresql") &&
        (dbtype != "cql")) {
        isc_throw(BadValue, "unknown backend database type: " << dbtype
                  << " (" << database_config->getPosition() << ")");
    }

I have two concerns. First, the check is not always precise. Postgres, MySQL
and Cassandra are all optional features, so they may or may not be
valid values.

The second concern is more fundamental. There may be
users who develop their own database backends. We know that, because
Cassandra was developed externally and only contributed later.
With this check, maintaining such a patch would be more difficult.

Therefore I think this check should be removed, or at least converted
to a warning, not an exception, which means a definite failure.

Otherwise your changes are good.

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

comment:11 in reply to: ↑ 10 Changed 3 years ago by fdupont

Replying to tomek:

Oops, I modified the ticket incorrectly. Fixing it now.

dbaccess_parser.cc
I have some doubts about this code:

    if ((dbtype != "memfile") &&
        (dbtype != "mysql") &&
        (dbtype != "postgresql") &&
        (dbtype != "cql")) {
        isc_throw(BadValue, "unknown backend database type: " << dbtype
                  << " (" << database_config->getPosition() << ")");
    }

=> first the check was there so it is far to be new. Second the grammar accepts
only one of these four values so this check enters in the "we check this
condition but it will be never met with input from flex/bison".

I have two concerns. First, the check is not always precise. Postgres, MySQL
and Cassandra are all optional features, so they may or may not be
valid values.

=> I had the same concern when I migrated the code but as it seemed
to be right before I considered it was not enough to change it.

The second concern is more fundamental. There may be
users who develop their own database backends. We know that, because
Cassandra was developed externally and only contributed later.
With this check, maintaining such a patch would be more difficult.

=> yes, at least there should be a comment saying it is questionable
and referring to the grammar so to add a new DB backend will be easier.

Therefore I think this check should be removed, or at least converted
to a warning, not an exception, which means a definite failure.

=> as I said it is not enough to remove the check so IMHO it is better
to document how to add a new DB backend. I suggest to create a ticket
for this as parser migration made this even harder.

Otherwise your changes are good.

=> so I'll merge as soon as we reach a consensus about what to do
for the check. BTW I'll take the occasion to use the position of
subtype (vs database_config).

comment:12 Changed 3 years ago by fdupont

  • Status changed from reviewing to accepted

comment:13 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek
  • Status changed from accepted to reviewing

Improved a bit the code, in particular added a comment about new DB backends.

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

  • Add Hours to Ticket changed from 2 to 1
  • Owner changed from tomek to fdupont
  • Total Hours changed from 2 to 1

Thanks a lot for your explanations regarding the database type checks.
I think one day we will change the grammar to also allow a generic
string for database type and print a warning if type is uknown.
People who develop their own backends are skilled with databases,
but they don't necessarily must understand how bison grammar works.
Anyway, this is a minor future improvement, so there's no need to
spend any more time on this.

I have reviewed your latest changes. They're good, but I have
one couple minor comment. In mysql_connection.c the default
value (MYSQL_DEFAULT_CONNECTION_TIMEOUT) should be
defined in src/bin/dhcpX/simple_parserX.cc with all the other
default values. This is outside of scope for this ticket, so
I added a @todo for now. The same comment applies to
pgsql_connection.c.

Please pull and check if you're ok with
those todos. If you are, this ticket is ready to go.

comment:15 in reply to: ↑ 14 Changed 3 years ago by fdupont

Replying to tomek:

Thanks a lot for your explanations regarding the database type checks.
I think one day we will change the grammar to also allow a generic
string for database type and print a warning if type is uknown.
People who develop their own backends are skilled with databases,
but they don't necessarily must understand how bison grammar works.
Anyway, this is a minor future improvement, so there's no need to
spend any more time on this.

=> IMHO there should be a document explaining how to add
a new database backend...

I have reviewed your latest changes. They're good, but I have
one couple minor comment. In mysql_connection.c the default
value (MYSQL_DEFAULT_CONNECTION_TIMEOUT) should be
defined in src/bin/dhcpX/simple_parserX.cc with all the other
default values. This is outside of scope for this ticket, so
I added a @todo for now. The same comment applies to
pgsql_connection.c.

=> I agree for the @todo's but the target should not be

src/bin/dhcpX/simple_parserX.cc. Now it does not matter

so I am merging with current @todo's.

comment:16 Changed 3 years ago by fdupont

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

Merged. Closing...

Note: See TracTickets for help on using tickets.