Copilot commented on code in PR #7246:
URL: https://github.com/apache/ignite-3/pull/7246#discussion_r2626774737
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientHandlerModule.java:
##########
@@ -176,6 +178,7 @@ public class ClientHandlerModule implements
IgniteComponent, PlatformComputeTran
* @param clientConnectorConfiguration Configuration of the connector.
* @param lowWatermark Low watermark.
* @param partitionOperationsExecutor Executor for a partition operation.
+ * @param ddlBatchingSuggestionEnabled Boolean configuration value
indicates whether the suggestion related DDL batching is enabled.
Review Comment:
The JavaDoc parameter description is incomplete. Similar to the issue in
ClientInboundMessageHandler, it should specify that this is a "Supplier" that
provides the configuration value, rather than describing it as a "Boolean
configuration value".
```suggestion
* @param ddlBatchingSuggestionEnabled Supplier that provides the
configuration value indicating whether the suggestion related DDL batching is
enabled.
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientDdlQueriesTrackerTest.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.runner.app.client;
+
+import static org.apache.ignite.internal.TestWrappers.unwrapIgniteImpl;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.await;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.sameInstance;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.client.handler.ClientInboundMessageHandler;
+import
org.apache.ignite.internal.configuration.SuggestionsClusterExtensionConfiguration;
+import org.apache.ignite.internal.lang.IgniteStringFormatter;
+import org.apache.ignite.internal.util.IgniteUtils;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * End-to-end tests to validate the behavior of the DDL queries suggestion
handler.
+ */
+public class ItThinClientDdlQueriesTrackerTest extends
ItAbstractThinClientTest {
+ private final List<AutoCloseable> closeables = new ArrayList<>();
+
+ @Override
+ protected int nodes() {
+ return 1;
+ }
+
+ @AfterEach
+ void tearDown() throws Exception {
+ server(0).sql().executeScript("DROP TABLE IF EXISTS t");
+
+ IgniteUtils.closeAll(closeables);
+ }
+
+ @Test
+ void ddlIsTrackedByConnection() {
+ server(0).sql().executeScript("CREATE TABLE t(id INT PRIMARY KEY)");
+
+ IgniteClient client1 = client();
+ ClientInboundMessageHandler handler1 =
unwrapIgniteImpl(server(0)).clientInboundMessageHandler();
+
+ // The handler "test reference" is updated after a new connection is
established.
+ IgniteClient client2 = startClient();
+ ClientInboundMessageHandler handler2 =
unwrapIgniteImpl(server(0)).clientInboundMessageHandler();
Review Comment:
The test assumes that calling
`unwrapIgniteImpl(server(0)).clientInboundMessageHandler()` after establishing
a connection returns the handler for that specific connection. However, this
approach is fragile and relies on the timing of when the handler reference is
obtained. The handler reference might not be the correct one if there's any
delay or if multiple connections exist. Consider storing a reference to each
connection's handler at the time of connection establishment, or use a more
reliable way to track which handler corresponds to which client.
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -290,6 +294,7 @@ public class ClientInboundMessageHandler
* @param partitionOperationsExecutor Partition operations executor.
* @param features Features.
* @param extensions Extensions.
+ * @param ddlBatchingSuggestionEnabled Boolean configuration value
indicates whether the suggestion related DDL batching is enabled.
Review Comment:
The JavaDoc parameter description is incomplete. It should be more
consistent with other parameter descriptions in this constructor and specify
that this is a "Supplier" that indicates whether the suggestion is enabled,
rather than describing it as a "Boolean configuration value".
```suggestion
* @param ddlBatchingSuggestionEnabled Supplier that indicates whether
the suggestion related to DDL batching is enabled.
```
##########
modules/configuration-system/src/main/java/org/apache/ignite/internal/configuration/SuggestionsClusterExtensionConfigurationSchema.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.configuration;
+
+import org.apache.ignite.configuration.annotation.ConfigValue;
+import org.apache.ignite.configuration.annotation.ConfigurationExtension;
+
+/**
+ * Extension for suggestions distributed endpoint subtree.
Review Comment:
The JavaDoc comment says "distributed endpoint subtree" but it should be
more specific and say "cluster-wide configuration" to be consistent with
similar configuration extension classes in the codebase.
```suggestion
* Extension for suggestions cluster-wide configuration.
```
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/DdlBatchingSuggester.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.client.handler;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Consumer;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.apache.ignite.internal.sql.engine.SqlQueryType;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * Tracks the number of consecutive DDL queries executed and prints
+ * the recommendation to use batch processing of DDL queries.
+ */
+public class DdlBatchingSuggester implements Consumer<SqlQueryType> {
+ /** The number of DDL commands, after which it is necessary to print the
recommendation for the user. */
+ static final int THRESHOLD = 10;
+
+ /** Logger. */
+ private static final IgniteLogger LOG =
Loggers.forClass(DdlBatchingSuggester.class);
+
+ /** Message printer. */
+ private final Consumer<String> printer;
+
+ /** Number of DDL queries processed in a row. */
+ private final AtomicInteger counter = new AtomicInteger();
+
+ DdlBatchingSuggester() {
+ this.printer = LOG::warn;
+ }
+
+ @TestOnly
+ DdlBatchingSuggester(Consumer<String> printer) {
+ this.printer = printer;
+ }
+
+ @Override
+ public void accept(SqlQueryType type) {
+ if (type != SqlQueryType.DDL) {
+ counter.set(0);
+
+ return;
+ }
+
+ if (counter.incrementAndGet() == THRESHOLD) {
+ printer.accept("The system detected that a batch of DDL commands
was executed one by one. "
+ + "For better performance, it is recommended to combine
DDL commands into a single SQL script.");
+ }
+ }
Review Comment:
There is a race condition in this method when handling DDL queries
concurrently. If multiple threads execute DDL queries at the same time, they
could all read the counter value as THRESHOLD-1, increment it, and then all
observe it as exactly equal to THRESHOLD at line 61. This would cause the
suggestion to be printed multiple times instead of exactly once. Consider using
`compareAndSet` or a separate AtomicBoolean flag to ensure the message is
printed exactly once when the threshold is reached.
--
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]
