You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/04/01 12:32:15 UTC
[GitHub] [guacamole-client] DouglasHeriot opened a new pull request #488:
GUACAMOLE-919 defaultStatementTimeout and socketTimeout
DouglasHeriot opened a new pull request #488: GUACAMOLE-919 defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488
Pass through environment variables to set `defaultStatementTimeout` and `socketTimeout`. See issue GUACAMOLE-919 for details.
If the TCP connection to the database is silently dropped (not closed), doing a ping query "SELECT 1;" will hang indefinitely. The socketTimeout option will put a timeout on this.
`defaultStatementTimeout` is not relevant to this issue. I thought it might have been at first. I've left this in incase having the option exposed is useful for other reasons.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] DouglasHeriot commented on a change in pull
request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout
and socketTimeout
Posted by GitBox <gi...@apache.org>.
DouglasHeriot commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r402000471
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
Review comment:
Yes it takes a string value. I thought that just keeping it as a string was neater than making a nullable `Integer` where all other properties are `int`.
I've now changed this to an `int` and don't pass through when it's `0`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] necouchman commented on a change in pull request
#488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and
socketTimeout
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r402704371
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
Review comment:
I dug into this a little more, and it does not look to me like the string value "null" is what they intend - I think just mean `null` in the since of not setting anything. So, this should not be `"null"`, it should be `null`.
https://mybatis.org/mybatis-3/configuration.html
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] necouchman commented on a change in pull request
#488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and
socketTimeout
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r402704371
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
Review comment:
I dug into this a little more, and it does not look to me like the string value "null" is what they intend - I think just mean `NULL` in the since of not setting anything. So, this should not be `"null"`, it should be `NULL`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] necouchman commented on a change in pull request
#488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and
socketTimeout
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r401714918
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
Review comment:
`"null"` != `NULL` - does the underlying setting require the String value `null` for this?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] DouglasHeriot commented on a change in pull
request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout
and socketTimeout
Posted by GitBox <gi...@apache.org>.
DouglasHeriot commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r402733711
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
Review comment:
@necouchman I agree, it looks like I made a mistake. See my latest commit on this branch for the fix 330b2c3ec0c1fa5ed62f4efcb8585f7c287f86c9
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] DouglasHeriot commented on issue #488:
GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
Posted by GitBox <gi...@apache.org>.
DouglasHeriot commented on issue #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#issuecomment-607567859
These properties are passed in as string properties. `"null"` means not-set for `defaultStatementTimeout` however `0` is not-set for `socketTimeout`. I initially decided to try and just keep the properties as strings, but `int` did make more sense for `socketTimeout`.
https://mybatis.org/mybatis-3/configuration.html
https://jdbc.postgresql.org/documentation/head/connect.html
I agree int is nicer, so to keep consistency I have now made `defaultStatementTimeout` and `int` as well, and simply don't set the property on the backend if it's `0` to match the behaviour of `socketTimeout`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] necouchman commented on a change in pull request
#488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and
socketTimeout
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r401713600
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
+
+ /**
+ * The default socketTimeout (in seconds), if POSTGRESQL_SOCKET_TIMEOUT is not specified.
+ * Default to 0 (no timeout)
+ * https://jdbc.postgresql.org/documentation/head/connect.html
+ */
+ private static final int DEFAULT_SOCKET_TIMEOUT = 0;
+
Review comment:
Is there some reason why you've implemented one of these as a `String` and the other as an `int`? I think I would stick with `int` to be consistent with how other timeouts are implemented. Also, one uses `null` for no timeout, the other uses 0 - can we align on that, or does the underlying code note allow for that?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] necouchman commented on a change in pull request
#488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and
socketTimeout
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r402704371
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
Review comment:
I dug into this a little more, and it does not look to me like the string value "null" is what they intend - I think just mean `NULL` in the since of not setting anything. So, this should not be `"null"`, it should be `NULL`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] necouchman commented on a change in pull request
#488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and
socketTimeout
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r402704371
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
Review comment:
I dug into this a little more, and it does not look to me like the string value "null" is what they intend - I think just mean `NULL` in the since of not setting anything. So, this should not be `"null"`, it should be `NULL`.
https://mybatis.org/mybatis-3/configuration.html
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] DouglasHeriot commented on a change in pull
request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout
and socketTimeout
Posted by GitBox <gi...@apache.org>.
DouglasHeriot commented on a change in pull request #488: GUACAMOLE-919: Implement PostgreSQL defaultStatementTimeout and socketTimeout
URL: https://github.com/apache/guacamole-client/pull/488#discussion_r402000686
##########
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 null (no timeout)
+ * https://mybatis.org/mybatis-3/configuration.html
+ */
+ private static final String DEFAULT_DEFAULT_STATEMENT_TIMEOUT = "null";
+
+ /**
+ * The default socketTimeout (in seconds), if POSTGRESQL_SOCKET_TIMEOUT is not specified.
+ * Default to 0 (no timeout)
+ * https://jdbc.postgresql.org/documentation/head/connect.html
+ */
+ private static final int DEFAULT_SOCKET_TIMEOUT = 0;
+
Review comment:
Yes, I was trying to keep same as what the backend takes. But I see your point - consistency at this level would be good rather than exposing the underlying differences.
----------------------------------------------------------------
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
With regards,
Apache Git Services