[#3542] Added additional parameters to the JDBC configuration #3548

mattkaem · 2023-09-25T12:34:38Z

Additional parameters are:

minimumPoolSize
initialPoolSize
maximumIdleTime

sophokles73 · 2023-10-04T08:44:22Z

site/documentation/content/admin-guide/jdbc-device-registry-config.md

@@ -66,12 +66,18 @@ and availability.
 | `HONO_REGISTRY_JDBC_ADAPTER_USERNAME`            <br> `hono.registry.jdbc.adapter.username`             | no  | -    | The username used to access the database. |
 | `HONO_REGISTRY_JDBC_ADAPTER_PASSWORD`            <br> `hono.registry.jdbc.adapter.password`             | no  | -    | The password used to access the database. |
 | `HONO_REGISTRY_JDBC_ADAPTER_MAXIMUMPOOLSIZE`     <br> `hono.registry.jdbc.adapter.maximumPoolSize`      | no  | Depends on the connection pool implementation. `15` for C3P0. | The maximum size of the connection pool. |
+| `HONO_REGISTRY_JDBC_ADAPTER_MINIMUMPOOLSIZE`     <br> `hono.registry.jdbc.adapter.minimumPoolSize`      | no  | Depends on the connection pool implementation. `3` for C3P0.   | The minimum size of the connection pool. |


since there is no way for a user to configure the particular connection pool implementation to be used, I believe that we should remove the reference to C3P0 here (and in the other places). I would also propose to define reasonable default values for the arbitrary properties in the JdbcOptions class itself, instead of relying on the implicit defaults of C3P0 ...

I removed the references and defined default values in the JdbcOptions class.

sophokles73 · 2023-10-10T06:12:35Z

services/base-jdbc/src/main/java/org/eclipse/hono/service/base/jdbc/config/JdbcProperties.java

+        options.maximumPoolSize().ifPresentOrElse(this::setMaximumPoolSize, () -> setMaximumPoolSize(MAXIMUM_POOL_SIZE_DEFAULT));
+        options.minimumPoolSize().ifPresentOrElse(this::setMinimumPoolSize, () -> setMinimumPoolSize(MINIMUM_POOL_SIZE_DEFAULT));
+        options.initialPoolSize().ifPresentOrElse(this::setInitialPoolSize, () -> setInitialPoolSize(INITIAL_POOL_SIZE_DEFAULT));
+        options.maximumIdleTime().ifPresentOrElse(this::setMaximumIdleTime, () -> setMaximumIdleTime(MAXIMUM_IDLE_TIME_DEFAULT));


we usually set the default values in the corresponding *Options class methods using the @WithDefault annotation. For an example, you might want to refer to https://github.com/eclipse-hono/hono/blob/master/core/src/main/java/org/eclipse/hono/config/ServerOptions.java

I changed it. Thanks for the tip :)

sophokles73 · 2023-10-10T06:14:26Z

services/base-jdbc/src/main/java/org/eclipse/hono/service/base/jdbc/config/JdbcProperties.java

+        if (dataSourceProperties.getMinimumPoolSize() != null) {
+            config.put("min_pool_size", dataSourceProperties.getMinimumPoolSize());
+        }
+        if (dataSourceProperties.getInitialPoolSize() != null) {


should we also add a check whether the initial pool size is indeed between min and max?

I thought about this as well, but decided against it because, since every value could be null (via default constructor or setter methods), it would be very complicated to cover every edge case meaningfully.

when we change the types of these properties to int, there shouldn't be an issue with checking the boundaries anymore, right?

yes, but then we would have to get rid of the default constructor or add the default values there as well.

Yes, that is what we do in other properties classes as well, e.g. ServiceConfigProperties. This is not pretty but at least it would be consistent. The overall issue is that we had introduced the Options/Properties duality during the time of the migration from Spring to Quarkus. Spring heavily uses properties, Quarkus uses the interface based Options approach. We will need to start tidying that up ...

ok, I was already wondering why that is. I added the defaults to the Properties class as well, changed the types from Integer to int and added a value validation.

sophokles73 · 2023-10-10T06:15:27Z

services/base-jdbc/src/main/java/org/eclipse/hono/service/base/jdbc/config/JdbcOptions.java

+    OptionalInt minimumPoolSize();
+
+    /**
+     * Gets the initial size of the DB connection pool.
+     *
+     * @return The initial number of connections in the pool.
+     */
+    OptionalInt initialPoolSize();
+
+    /**
+     * Gets the maximum idle time of connections in the DB connection pool.
+     *
+     * @return The maximum idle time of connections in the pool.
+     */
+    OptionalInt maximumIdleTime();


should we add checks to ensure the values are >= 0?

sure, we could do that. Is there an annotation or something you have in mind that I should use or why did you comment this in this interface rather than in the JdbcProperties class?

sophokles73 · 2023-10-30T17:04:17Z

services/base-jdbc/src/main/java/org/eclipse/hono/service/base/jdbc/config/JdbcProperties.java

@@ -38,6 +38,9 @@ public class JdbcProperties {
    private String username;
    private String password;
    private Integer maximumPoolSize;
+    private Integer minimumPoolSize;


now that we always have a (default) value for these properties, we should probably define them as ints instead of Integers, shouldn't we?

sophokles73 · 2023-10-30T17:05:17Z

services/base-jdbc/src/main/java/org/eclipse/hono/service/base/jdbc/config/JdbcProperties.java

+        if (dataSourceProperties.getMinimumPoolSize() != null) {
+            config.put("min_pool_size", dataSourceProperties.getMinimumPoolSize());
+        }
+        if (dataSourceProperties.getInitialPoolSize() != null) {


when we change the types of these properties to int, there shouldn't be an issue with checking the boundaries anymore, right?

…mIdleTime as parameters to the JDBC configuration Signed-off-by: Matthias Kaemmer <[email protected]>

removed reference to c3p0 and set default values for the connection pool in the JdbcProperties class Signed-off-by: Matthias Kaemmer <[email protected]>

changed declaration of default values for connection pool Signed-off-by: Matthias Kaemmer <[email protected]>

* removed null checks * added value validation Signed-off-by: Matthias Kaemmer <[email protected]>

sophokles73 · 2023-11-03T06:57:15Z

services/base-jdbc/src/main/java/org/eclipse/hono/service/base/jdbc/config/JdbcProperties.java

@@ -140,4 +179,16 @@ public static JDBCClient dataSource(final Vertx vertx, final JdbcProperties data

    }

+    private static void putValidValueIntoConfig(final JsonObject config, final String label, final int value, final int limit, final boolean checkLowerLimit) {


Given that we have defined the limits at the class level, by means of constants, I believe that we should enforce these limits in the setters instead of deferring it to the time the datasource is being constructed (which may actually never happen), or am I mistaken?

For the non negativ checks I think you are right, we could do those checks in the setter methods. However, for the initialPoolSize we would have the problem that we don't have control over the order in which someone calls the setter methods. So for example if you call setInitialPoolSize with input 5, which is between the defaults for min and maxPoolSize, this would be seen as valid input and initialPoolSize would be set to 5. After that you could just change the maxPoolSize to be below 5 or the minPoolSize to be above 5, which would result in an invalid configuration. Therefor, in my opinion it makes more sense to enforce this right before we create the JDBCClient, at which point we can be sure all the values have been set. What do you think?

Ok, I see your point

sophokles73

LGTM

sophokles73 · 2023-11-04T16:17:15Z

services/base-jdbc/src/main/java/org/eclipse/hono/service/base/jdbc/config/JdbcProperties.java

@@ -140,4 +179,16 @@ public static JDBCClient dataSource(final Vertx vertx, final JdbcProperties data

    }

+    private static void putValidValueIntoConfig(final JsonObject config, final String label, final int value, final int limit, final boolean checkLowerLimit) {


Ok, I see your point

mattkaem added the Device Registry label Sep 25, 2023

mattkaem requested a review from kaniyan as a code owner September 25, 2023 12:34

mattkaem self-assigned this Sep 25, 2023

mattkaem requested a review from sophokles73 as a code owner September 25, 2023 12:34

mattkaem added JDBC Device Registry The JDBC based Device Registry implementation and removed Device Registry labels Sep 25, 2023

sophokles73 reviewed Oct 4, 2023

View reviewed changes

sophokles73 added this to the 2.5.0 milestone Oct 4, 2023

sophokles73 reviewed Oct 10, 2023

View reviewed changes

harism mentioned this pull request Oct 15, 2023

[#3562] Migrate to Quarkus JDBC implementation #3563

Merged

sophokles73 requested changes Oct 30, 2023

View reviewed changes

mattkaem added 3 commits November 2, 2023 12:27

[eclipse-hono#3542] Added minimumPoolSize, initialPoolSize and maximu…

cc2ae12

…mIdleTime as parameters to the JDBC configuration Signed-off-by: Matthias Kaemmer <[email protected]>

Review fixes:

a71cbe7

removed reference to c3p0 and set default values for the connection pool in the JdbcProperties class Signed-off-by: Matthias Kaemmer <[email protected]>

Review fixes:

101c3fb

changed declaration of default values for connection pool Signed-off-by: Matthias Kaemmer <[email protected]>

mattkaem linked an issue Nov 2, 2023 that may be closed by this pull request

Add configuration parameters for JDBC connection pool #3542

Closed

Review fixes:

bca7159

* removed null checks * added value validation Signed-off-by: Matthias Kaemmer <[email protected]>

mattkaem force-pushed the add-configuration-paramters-for-jdbc branch from 9c93861 to bca7159 Compare November 2, 2023 12:51

sophokles73 reviewed Nov 3, 2023

View reviewed changes

sophokles73 approved these changes Nov 4, 2023

View reviewed changes

sophokles73 merged commit a2def04 into eclipse-hono:master Nov 4, 2023
8 checks passed

mattkaem deleted the add-configuration-paramters-for-jdbc branch November 6, 2023 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#3542] Added additional parameters to the JDBC configuration #3548

[#3542] Added additional parameters to the JDBC configuration #3548

mattkaem commented Sep 25, 2023

sophokles73 Oct 4, 2023

mattkaem Oct 9, 2023

sophokles73 Oct 10, 2023

mattkaem Oct 30, 2023

sophokles73 Oct 10, 2023

mattkaem Oct 30, 2023

sophokles73 Oct 30, 2023

mattkaem Oct 31, 2023

sophokles73 Nov 1, 2023

mattkaem Nov 2, 2023

sophokles73 Oct 10, 2023

mattkaem Oct 30, 2023

sophokles73 Oct 30, 2023

sophokles73 Oct 30, 2023

sophokles73 Nov 3, 2023

mattkaem Nov 3, 2023

sophokles73 Nov 4, 2023

sophokles73 left a comment

sophokles73 Nov 4, 2023

		@@ -140,4 +179,16 @@ public static JDBCClient dataSource(final Vertx vertx, final JdbcProperties data

		}

		private static void putValidValueIntoConfig(final JsonObject config, final String label, final int value, final int limit, final boolean checkLowerLimit) {

[#3542] Added additional parameters to the JDBC configuration #3548

[#3542] Added additional parameters to the JDBC configuration #3548

Conversation

mattkaem commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment