zstan commented on code in PR #12758:
URL: https://github.com/apache/ignite/pull/12758#discussion_r2820568271


##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/MemoryQuotasIntegrationTest.java:
##########
@@ -377,6 +377,20 @@ public void testHashAggregateNode() {
             IgniteException.class, "Query quota exceeded");
     }
 
+    /** */
+    @Test
+    public void testMassiveSequentialCheck() {
+        sql("CREATE TABLE tbl2 (id INT, b VARBINARY) WITH 
TEMPLATE=PARTITIONED");
+
+        for (int i = 0; i < 2000; i++)
+            sql("INSERT INTO tbl2 VALUES (?, ?)", i, new byte[1000]);
+
+        for (int i = 0; i < 1000; i++) {
+            assertThrows("SELECT ANY_VALUE(b) FROM tbl2 GROUP BY id",

Review Comment:
   seems you can reuse **tbl** here, not fill additional **tbl2**
   ```suggestion
               assertThrows("SELECT ANY_VALUE(b) FROM tbl GROUP BY id",
   ```



##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/MemoryQuotasIntegrationTest.java:
##########
@@ -377,6 +377,20 @@ public void testHashAggregateNode() {
             IgniteException.class, "Query quota exceeded");
     }
 
+    /** */
+    @Test
+    public void testMassiveSequentialCheck() {
+        sql("CREATE TABLE tbl2 (id INT, b VARBINARY) WITH 
TEMPLATE=PARTITIONED");
+
+        for (int i = 0; i < 2000; i++)
+            sql("INSERT INTO tbl2 VALUES (?, ?)", i, new byte[1000]);
+
+        for (int i = 0; i < 1000; i++) {
+            assertThrows("SELECT ANY_VALUE(b) FROM tbl2 GROUP BY id",
+                IgniteException.class, "Query quota exceeded");
+        }

Review Comment:
   I don`t like sequential loop tests approach it consume extra time and 
highlights potential flaky point, seems just such a code will be enough here, 
it passed for me numerous times. If you decide that it can be flaky - different 
test without loop need to be here.
   ```suggestion
               assertThrows("SELECT ANY_VALUE(b) FROM tbl GROUP BY id",
                   IgniteException.class, "Query quota exceeded");
   ```



##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/MemoryQuotasIntegrationTest.java:
##########
@@ -377,6 +377,20 @@ public void testHashAggregateNode() {
             IgniteException.class, "Query quota exceeded");
     }
 
+    /** */
+    @Test
+    public void testMassiveSequentialCheck() {
+        sql("CREATE TABLE tbl2 (id INT, b VARBINARY) WITH 
TEMPLATE=PARTITIONED");
+
+        for (int i = 0; i < 2000; i++)
+            sql("INSERT INTO tbl2 VALUES (?, ?)", i, new byte[1000]);
+
+        for (int i = 0; i < 1000; i++) {
+            assertThrows("SELECT ANY_VALUE(b) FROM tbl2 GROUP BY id",
+                IgniteException.class, "Query quota exceeded");

Review Comment:
   Seems it not related to an issue but i wonder - why  IgniteException is 
raised here ? Quotas is a part of sql - so some kind of IgniteSQLException need 
here ? 



##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/tracker/MemoryTrackerTest.java:
##########
@@ -136,14 +136,29 @@ public void testRemoveOverflow() {
     /** */
     @Test
     public void testConcurrentModification() throws Exception {
-        MemoryTracker globalTracker = new GlobalMemoryTracker(10_000_000L);
-        MemoryTracker qryTracker = new QueryMemoryTracker(globalTracker, 
1_000_000L);
+        MemoryTracker globalTracker = new GlobalMemoryTracker(8_000L);
+
+        MemoryTracker[] qryTrackers = new MemoryTracker[2];
+
+        for (int i = 0; i < qryTrackers.length; i++)
+            qryTrackers[i] = new QueryMemoryTracker(globalTracker, 5_000L);
+
         AtomicBoolean stop = new AtomicBoolean();
 
         IgniteInternalFuture<?> fut = GridTestUtils.runMultiThreadedAsync(() 
-> {
             while (!stop.get()) {
-                qryTracker.onMemoryAllocated(1_000L);
-                qryTracker.onMemoryReleased(1_000L);
+                MemoryTracker qryTracker = 
qryTrackers[ThreadLocalRandom.current().nextInt(qryTrackers.length)];
+
+                try {
+                    qryTracker.onMemoryAllocated(1_000L);
+                }
+                catch (Exception ignore) {
+                    // No-op.
+                }
+                finally {
+                    // Release a little bit more than allocated, to test 
inaccuracy of row size calculation.
+                    qryTracker.onMemoryReleased(1_001L);

Review Comment:
   it need to be strict consistency between allocated and released, otherwise 
inaccuracy of aclculation will be hide, but it also not related to an issue, 
just wonder how it works



-- 
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]

Reply via email to