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:
[email protected]
