mike-jumper commented on code in PR #821:
URL: https://github.com/apache/guacamole-client/pull/821#discussion_r1158838034
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/base/ModeledDirectoryObjectService.java:
##########
@@ -387,26 +398,38 @@ public Collection<InternalType>
retrieveObjects(ModeledAuthenticatedUser user,
Collection<String> identifiers) throws GuacamoleException {
// Ignore invalid identifiers
- identifiers = filterIdentifiers(identifiers);
+ List<String> filteredIdentifiers = filterIdentifiers(identifiers);
// Do not query if no identifiers given
- if (identifiers.isEmpty())
+ if (filteredIdentifiers.isEmpty())
return Collections.<InternalType>emptyList();
- Collection<ModelType> objects;
+ int batchSize = environment.getDefaultBatchSize();
- // Bypass permission checks if the user is privileged
- if (user.isPrivileged())
- objects = getObjectMapper().select(identifiers);
+ boolean userIsPrivileged = user.isPrivileged();
+
+ // Process the filteredIdentifiers in batches using IntStream.range
and flatMap
+ Collection<ModelType> allObjects = IntStream.range(0,
filteredIdentifiers.size())
+ .filter(index -> index % batchSize == 0)
Review Comment:
While I do like Java's fancy streams, this seems an inefficient way to
generate the starting indices of batches. I think we should directly calculate
the starting indices rather than doing a brute-force search through all indices.
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-mysql/src/main/java/org/apache/guacamole/auth/mysql/conf/MySQLEnvironment.java:
##########
@@ -104,6 +104,12 @@ public class MySQLEnvironment extends JDBCEnvironment {
* The default SSL mode for connecting to MySQL servers.
*/
private final MySQLSSLMode DEFAULT_SSL_MODE = MySQLSSLMode.PREFERRED;
+
+ /**
+ * The default maximum number of identifiers/parameters to be included in
a
+ * single batch when executing SQL statements.
+ */
+ private static final int DEFAULT_BATCH_SIZE = 2147483647;
Review Comment:
Is there documentation that might be referenced here that shows `2147483647`
is the relevant limit for both MySQL and MariaDB?
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/conf/PostgreSQLEnvironment.java:
##########
@@ -115,6 +115,12 @@ public class PostgreSQLEnvironment extends JDBCEnvironment
{
* The default value to use for SSL mode if none is explicitly configured.
*/
private final PostgreSQLSSLMode DEFAULT_SSL_MODE =
PostgreSQLSSLMode.PREFER;
+
+ /*
+ * The default maximum number of identifiers/parameters to be included in
a
+ * single batch when executing SQL statements.
+ */
+ private static final int DEFAULT_BATCH_SIZE = 2147483647;
Review Comment:
Is there documentation that might be referenced here that shows `2147483647`
is the relevant limit for PostgreSQL?
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-sqlserver/src/main/java/org/apache/guacamole/auth/sqlserver/conf/SQLServerEnvironment.java:
##########
@@ -88,6 +88,12 @@ public class SQLServerEnvironment extends JDBCEnvironment {
*/
public static final SQLServerDriver SQLSERVER_DEFAULT_DRIVER =
SQLServerDriver.MICROSOFT_2005;
+ /**
+ * The default maximum number of identifiers/parameters to be included in
a
+ * single batch when executing SQL statements.
+ */
+ private static final int DEFAULT_BATCH_SIZE = 1000;
Review Comment:
Is there documentation that might be referenced here that shows `1000` is
the relevant limit for SQL Server?
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-postgresql/src/main/java/org/apache/guacamole/auth/postgresql/conf/PostgreSQLGuacamoleProperties.java:
##########
@@ -302,5 +302,17 @@ private PostgreSQLGuacamoleProperties() {}
public String getName() { return
"postgresql-enforce-access-windows-for-active-sessions"; }
};
+
+ /**
+ * The maximum number of identifiers/parameters to be included in a single
batch when
+ * executing SQL statements.
+ */
+ public static final IntegerGuacamoleProperty POSTGRES_BATCH_SIZE =
+ new IntegerGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "postgres-batch-size"; }
+
+ };
Review Comment:
Please maintain established naming convention (`postgresql-*` for the
property, `POSTGRESQL_*` for the constant).
##########
extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/JDBCEnvironment.java:
##########
@@ -67,6 +67,19 @@ public JDBCEnvironment() {
*/
public abstract int getAbsoluteMaxConnections() throws GuacamoleException;
+ /**
+ * Returns the default maximum number of identifiers/parameters to be
+ * included in a single batch when executing SQL statements.
+ *
+ * @return
+ * The default maximum number of identifiers/parameters to be included
+ * in a single batch.
+ *
+ * @throws GuacamoleException
+ * If an error occurs while retrieving the property.
+ */
+ public abstract int getDefaultBatchSize() throws GuacamoleException;
Review Comment:
Is it the default batch size? Or is it just the batch size?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
