necouchman commented on a change in pull request #488:
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r417504647
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/PostgreSQLEnvironment.java
##########
@@ -47,6 +47,21 @@
*/
private static final int DEFAULT_PORT = 5432;
+ /**
+ * The default defaultStatementTimeout (in seconds),
+ * if POSTGRESQL_DEFAULT_STATEMENT_TIMEOUT is not specified.
+ * Default to 0 (no timeout, property won't be set)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final int DEFAULT_DEFAULT_STATEMENT_TIMEOUT = 0;
Review comment:
I think we should probably keep it to one `DEFAULT` here :-). I see
what you were going for, but I don't think the extra `DEFAULT` is necessary -
it makes perfect sense (and, arguably, more sense) without it.
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/PostgreSQLAuthenticationProviderModule.java
##########
@@ -67,9 +67,18 @@ 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) {
Review comment:
Style issue that it took me a long time to learn (and I still get wrong
sometimes).
In the words of @mike-jumper, "if is not a function" - so should be `if
(defaultStatementTimeout > 0) {` (with a space between the `if` and the
condition block.
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/PostgreSQLGuacamoleProperties.java
##########
@@ -92,6 +92,30 @@ private PostgreSQLGuacamoleProperties() {}
};
+ /**
+ * Sets 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"; }
+
+ };
+
+ /**
+ * Sets the number of seconds the driver will wait in a read() call
Review comment:
As above, this isn't setting anything, so more direct language would
probably be better, here.
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/PostgreSQLGuacamoleProperties.java
##########
@@ -92,6 +92,30 @@ private PostgreSQLGuacamoleProperties() {}
};
+ /**
+ * Sets the number of seconds the driver will wait for
Review comment:
This is a bit of a nitpick, but the below statement doesn't actually
"set" anything. It is a property that can be used to set it. Take a look at
some of the other properties around this - for example, "The password used to
authenticate..." This can probably be simplified to something similar to that.
##########
File path:
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/PostgreSQLEnvironment.java
##########
@@ -47,6 +47,21 @@
*/
private static final int DEFAULT_PORT = 5432;
+ /**
+ * The default defaultStatementTimeout (in seconds),
+ * if POSTGRESQL_DEFAULT_STATEMENT_TIMEOUT is not specified.
+ * Default to 0 (no timeout, property won't be set)
+ * https://mybatis.org/mybatis-3/configuration.html
Review comment:
A couple things for this comment block:
* I'd avoid using the actual variable name (`defaultStatementTimeout`) and
just make it read like "The default statement timeout..."
* We don't really link to outside documentation for any of the other
properties, and I'm not sure it's worth starting for these two, so I'd remove
the links to external sites.
* For the explanation for "Default to 0," something more verbose might be
appropriate: "Setting this value to 0 (the default) will result in..."
----------------------------------------------------------------
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]
