[GitHub] [guacamole-client] necouchman commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout

[GitBox <gi...@apache.org>]

necouchman commented on a change in pull request #488:
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r451499471



##########
File path: guacamole-docker/bin/start.sh
##########
@@ -354,10 +354,18 @@ END
         "postgresql-default-max-group-connections-per-user" \
         "$POSTGRES_DEFAULT_MAX_GROUP_CONNECTIONS_PER_USER"
 
+    set_optional_property               \
+        "postgresql-default-statement-timeout" \

Review comment:
       Please line up the trailing `\`.

##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/conf/PostgreSQLGuacamoleProperties.java
##########
@@ -94,6 +94,30 @@ private PostgreSQLGuacamoleProperties() {}
 
     };
 
+    /**
+     * The number of seconds the driver will wait for
+     * a response from the database.
+     */
+    public static final IntegerGuacamoleProperty
+            POSTGRESQL_DEFAULT_STATEMENT_TIMEOUT = new IntegerGuacamoleProperty(){

Review comment:
       Please put a space between `()` and `{`.

##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/PostgreSQLAuthenticationProviderModule.java
##########
@@ -70,6 +70,12 @@ public PostgreSQLAuthenticationProviderModule(PostgreSQLEnvironment environment)
         myBatisProperties.setProperty("mybatis.pooled.pingEnabled", "true");
         myBatisProperties.setProperty("mybatis.pooled.pingQuery", "SELECT 1");
 
+        // Only set if > 0. Underlying backend does not take 0 as not-set.
+        int defaultStatementTimeout = environment.getPostgreSQLDefaultStatementTimeout();
+        if (defaultStatementTimeout > 0) {
+            myBatisProperties.setProperty("mybatis.configuration.defaultStatementTimeout", String.valueOf(defaultStatementTimeout));

Review comment:
       Might be good to break this line somewhere - this is a looooooooooooooong line :-).

##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/conf/PostgreSQLEnvironment.java
##########
@@ -48,6 +48,22 @@
      */
     private static final int DEFAULT_PORT = 5432;
 
+    /**
+     * The default number of seconds the driver will wait for a response from
+     * the database. A value of 0 (the default) means the timeout is disabled.
+     */
+    private static final int DEFAULT_STATEMENT_TIMEOUT = 0;
+
+    /**
+     * The default timeout (in seconds) used for socket read operations.
+     * If reading from the server takes longer than this value, the
+     * connection is closed. This can be used to handle network problems
+     * such as a dropped connection to the database. Similar to 
+     * DEFAULT_STATEMENT_TIMEOUT, it will also abort queries that take too 

Review comment:
       Two comments, here:
   * You say "similar to DEFAULT_STATEMENT_TIMEOUT", but there's no mention of aborting queries on the above timeout.
   * It looks like most of the documentation around what these two additional parameters do is here in the `DEFAULT_` sections, rather than down in the `PostgreSQLGuacamoleProperties.java` file.  If you look at the documentation of other `DEFAULT_`s around this, you'll see that the documentation is more succinct and references the actual properties.  I suggest doing the same with these two.

##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/conf/PostgreSQLGuacamoleProperties.java
##########
@@ -94,6 +94,30 @@ private PostgreSQLGuacamoleProperties() {}
 
     };
 
+    /**
+     * The number of seconds the driver will wait for
+     * a response from the database.
+     */
+    public static final IntegerGuacamoleProperty
+            POSTGRESQL_DEFAULT_STATEMENT_TIMEOUT = new IntegerGuacamoleProperty(){
+
+        @Override
+        public String getName() { return "postgresql-default-statement-timeout"; }
+
+    };
+
+    /**
+     * The number of seconds the driver will wait in a read() call
+     * on the TCP connection to the database.
+     */
+    public static final IntegerGuacamoleProperty
+            POSTGRESQL_SOCKET_TIMEOUT = new IntegerGuacamoleProperty(){

Review comment:
       Space between `()` and `{`, please.

##########
File path: guacamole-docker/bin/start.sh
##########
@@ -354,10 +354,18 @@ END
         "postgresql-default-max-group-connections-per-user" \
         "$POSTGRES_DEFAULT_MAX_GROUP_CONNECTIONS_PER_USER"
 
+    set_optional_property               \
+        "postgresql-default-statement-timeout" \
+        "$POSTGRES_DEFAULT_STATEMENT_TIMEOUT"
+
     set_optional_property          \
         "postgresql-user-required" \
         "$POSTGRES_USER_REQUIRED"
 
+    set_optional_property               \
+        "postgresql-socket-timeout" \

Review comment:
       Please line up the trailing `\`

##########
File path: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/conf/PostgreSQLEnvironment.java
##########
@@ -249,6 +265,41 @@ public String getPostgreSQLUsername() throws GuacamoleException {
     public String getPostgreSQLPassword() throws GuacamoleException {
         return getRequiredProperty(PostgreSQLGuacamoleProperties.POSTGRESQL_PASSWORD);
     }
+    
+    /**
+     * Returns the defaultStatementTimeout set for PostgreSQL connections.
+     * If unspecified, this will be the default 0,
+     * and should not be passed through to the backend.
+     * 
+     * @return
+     *     The statement timeout (in seconds)
+     *
+     * @throws GuacamoleException 
+     *     If an error occurs while retrieving the property value.
+     */
+    public int getPostgreSQLDefaultStatementTimeout() throws GuacamoleException {
+        return getProperty(
+            PostgreSQLGuacamoleProperties.POSTGRESQL_DEFAULT_STATEMENT_TIMEOUT,
+            DEFAULT_STATEMENT_TIMEOUT
+        );
+    }
+    
+    /**
+     * Returns the socketTimeout property to set on PostgreSQL connections.
+     * If unspecified, this will be the default to 0 (no timeout)

Review comment:
       Probably should either be:
   > this will default to
   
   or:
   > this will be the default of




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org