alex-plekhanov commented on code in PR #11425:
URL: https://github.com/apache/ignite/pull/11425#discussion_r1883714610
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/running/SqlPlanHistoryTracker.java:
##########
@@ -25,18 +25,13 @@
/** Class that manages recording and storing SQL plans. */
public class SqlPlanHistoryTracker {
/** SQL plan history. */
- private GridBoundedConcurrentLinkedHashMap<SqlPlan, Long> sqlPlanHistory;
-
- /** SQL plan history size. */
- private int historySize;
+ private Map<SqlPlan, Long> sqlPlanHistory;
/**
- * @param historySize SQL plan history size.
+ * @param histSize SQL plan history size.
*/
- public SqlPlanHistoryTracker(int historySize) {
- this.historySize = historySize;
-
- sqlPlanHistory = (historySize > 0) ? new
GridBoundedConcurrentLinkedHashMap<>(historySize) : null;
+ public SqlPlanHistoryTracker(int histSize) {
+ sqlPlanHistory = (histSize > 0) ? new
GridBoundedConcurrentLinkedHashMap<>(histSize) : Collections.emptyMap();
Review Comment:
`setHistorySize(histSize)` to reduce duplication?
##########
modules/core/src/main/java/org/apache/ignite/spi/systemview/view/SqlPlanHistoryView.java:
##########
@@ -34,6 +34,13 @@ public SqlPlanHistoryView(Map.Entry<SqlPlan, Long> plan) {
this.plan = plan;
}
+ /**
+ * @return SQL plan entry.
+ */
+ public Map.Entry<SqlPlan, Long> sqlPlan() {
Review Comment:
This method gives access to internals via public API. We should avoid this.
Method used only once in test method `getSqlPlanHistory`. In this method it's
more convinient to return something like `List<SqlPlanHistoryView>` than
`Map<SqlPlan, Long>`. In case of `List<SqlPlanHistoryView>` - all columns can
be checked thats exposed via public API (not only internal representation).
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java:
##########
@@ -291,141 +288,168 @@ public void testJdbcQueryFailed() throws Exception {
/** Checks successful SqlFieldsQuery. */
@Test
- public void testSqlFieldsQuery() {
+ public void testSqlFieldsQuery() throws Exception {
runSuccessfulQuery(sqlFieldsQry);
}
/** Checks failed SqlFieldsQuery. */
@Test
- public void testSqlFieldsQueryFailed() {
+ public void testSqlFieldsQueryFailed() throws Exception {
runFailedQuery(sqlFieldsQryFailed);
}
/** Checks successful cross-cache SqlFieldsQuery. */
@Test
- public void testSqlFieldsCrossCacheQuery() {
+ public void testSqlFieldsCrossCacheQuery() throws Exception {
runSuccessfulQuery(sqlFieldsQryCrossCache);
}
/** Checks failed cross-cache SqlFieldsQuery. */
@Test
- public void testSqlFieldsCrossCacheQueryFailed() {
+ public void testSqlFieldsCrossCacheQueryFailed() throws Exception {
runFailedQuery(sqlFieldsQryCrossCacheFailed);
}
- /** Checks successful SqlFieldsQuery with reduce phase. */
+ /**
+ * Checks successful SqlFieldsQuery with reduce phase. There should be 3
entries in SQL plan history on the
+ * reduce node (2 SELECT subqueries and 1 UNION/MERGE subquery) and 2
entries on the map node (2 SELECT subqueries).
+ */
@Test
- public void testSqlFieldsQueryWithReducePhase() {
+ public void testSqlFieldsQueryWithReducePhase() throws Exception {
runQueryWithReducePhase(() -> {
- cacheQuery(sqlFieldsQryWithReducePhase.setLocal(loc), "pers");
+ try {
+ startGridsMultiThreaded(1, 2);
+
+ awaitPartitionMapExchange();
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ cacheQuery(sqlFieldsQryWithReducePhase, "pers");
+
+ checkSqlPlanHistory(3);
- checkSqlPlanHistory((!isClient && sqlEngine ==
IndexingQueryEngineConfiguration.ENGINE_NAME) ? 3 : 1);
+ for (int i = 1; i <= 2; i++) {
+ Map<SqlPlan, Long> sqlPlansOnMapNode =
grid(i).context().query().runningQueryManager()
+ .planHistoryTracker().sqlPlanHistory();
+
+ assertNotNull(sqlPlansOnMapNode);
+
+ checkMetrics(2, sqlPlansOnMapNode);
+ }
});
}
- /** Checks failed SqlFieldsQuery with reduce phase. */
+ /**
+ * Checks failed SqlFieldsQuery with reduce phase. If the fisrt subquery
fails, there should be only one entry in
+ * SQL plan history. If the fisrt subquery is successfully executed but
the second one fails, the history should
Review Comment:
Typo: `fisrt`
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java:
##########
@@ -673,6 +720,50 @@ public void checkMetrics(int size, Map<SqlPlan, Long>
sqlPlans) {
}
}
+ /**
+ * Compares entries in the plan history before and after the reset event.
+ *
+ * @param reset Reset event.
+ */
+ public void checkReset(Runnable reset) throws Exception {
+ startTestGrid();
+
+ IgniteCache<Integer, String> cache = queryNode().cache("A");
+
+ String[] qryText = new String[2];
+
+ for (int i = 1; i <= 2; i++) {
+ cache.put(100 + i, "STR" + i);
+
+ cacheQuery(new SqlFieldsQuery(SQL + " where _val='STR" + i +
"'").setLocal(loc), "A");
+
+ qryText[i - 1] =
getSqlPlanHistory().keySet().stream().findFirst().map(SqlPlan::query).orElse("");
Review Comment:
Check history size?
F.first?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java:
##########
@@ -291,141 +288,168 @@ public void testJdbcQueryFailed() throws Exception {
/** Checks successful SqlFieldsQuery. */
@Test
- public void testSqlFieldsQuery() {
+ public void testSqlFieldsQuery() throws Exception {
runSuccessfulQuery(sqlFieldsQry);
}
/** Checks failed SqlFieldsQuery. */
@Test
- public void testSqlFieldsQueryFailed() {
+ public void testSqlFieldsQueryFailed() throws Exception {
runFailedQuery(sqlFieldsQryFailed);
}
/** Checks successful cross-cache SqlFieldsQuery. */
@Test
- public void testSqlFieldsCrossCacheQuery() {
+ public void testSqlFieldsCrossCacheQuery() throws Exception {
runSuccessfulQuery(sqlFieldsQryCrossCache);
}
/** Checks failed cross-cache SqlFieldsQuery. */
@Test
- public void testSqlFieldsCrossCacheQueryFailed() {
+ public void testSqlFieldsCrossCacheQueryFailed() throws Exception {
runFailedQuery(sqlFieldsQryCrossCacheFailed);
}
- /** Checks successful SqlFieldsQuery with reduce phase. */
+ /**
+ * Checks successful SqlFieldsQuery with reduce phase. There should be 3
entries in SQL plan history on the
+ * reduce node (2 SELECT subqueries and 1 UNION/MERGE subquery) and 2
entries on the map node (2 SELECT subqueries).
+ */
@Test
- public void testSqlFieldsQueryWithReducePhase() {
+ public void testSqlFieldsQueryWithReducePhase() throws Exception {
runQueryWithReducePhase(() -> {
- cacheQuery(sqlFieldsQryWithReducePhase.setLocal(loc), "pers");
+ try {
+ startGridsMultiThreaded(1, 2);
+
+ awaitPartitionMapExchange();
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ cacheQuery(sqlFieldsQryWithReducePhase, "pers");
+
+ checkSqlPlanHistory(3);
- checkSqlPlanHistory((!isClient && sqlEngine ==
IndexingQueryEngineConfiguration.ENGINE_NAME) ? 3 : 1);
+ for (int i = 1; i <= 2; i++) {
+ Map<SqlPlan, Long> sqlPlansOnMapNode =
grid(i).context().query().runningQueryManager()
+ .planHistoryTracker().sqlPlanHistory();
+
+ assertNotNull(sqlPlansOnMapNode);
+
+ checkMetrics(2, sqlPlansOnMapNode);
+ }
});
}
- /** Checks failed SqlFieldsQuery with reduce phase. */
+ /**
+ * Checks failed SqlFieldsQuery with reduce phase. If the fisrt subquery
fails, there should be only one entry in
+ * SQL plan history. If the fisrt subquery is successfully executed but
the second one fails, the history should
+ * contain two entries.
+ */
@Test
- public void testSqlFieldsQueryWithReducePhaseFailed() {
+ public void testSqlFieldsQueryWithReducePhaseFailed() throws Exception {
runQueryWithReducePhase(() -> {
- try {
- cacheQuery(sqlFieldsQryWithReducePhaseFailed.setLocal(loc),
"pers");
- }
- catch (Exception ignore) {
- //No-Op
- }
+ for (int i = 0; i < sqlFieldsQryWithReducePhaseFailed.length; i++)
{
+ try {
+ cacheQuery(sqlFieldsQryWithReducePhaseFailed[i], "pers");
+ }
+ catch (Exception ignore) {
+ // No-op.
+ }
+
+ checkSqlPlanHistory( i + 1);
Review Comment:
It's not intuitive clear that map-subqueries are executed one by one. It's
not stated anywhere and subqueries can be run in parallel with some tuning or
after some change in future. So, perhaps we should remove this test at all,
since can't assert correct behaviour.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java:
##########
@@ -218,15 +224,13 @@ protected IgniteEx queryNode() {
IgniteEx node = isClient ? grid(1) : grid(0);
if (isClient)
Review Comment:
Redundant
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/running/SqlPlanHistoryTracker.java:
##########
@@ -47,7 +42,7 @@ public SqlPlanHistoryTracker(int historySize) {
* @param engine SQL engine.
*/
public void addPlan(String plan, String qry, String schema, boolean loc,
String engine) {
- if (historySize <= 0)
+ if
(sqlPlanHistory.getClass().equals(Collections.emptyMap().getClass()))
Review Comment:
Why not just `sqlPlanHistory == Collections.emptyMap()`?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java:
##########
@@ -291,141 +288,168 @@ public void testJdbcQueryFailed() throws Exception {
/** Checks successful SqlFieldsQuery. */
@Test
- public void testSqlFieldsQuery() {
+ public void testSqlFieldsQuery() throws Exception {
runSuccessfulQuery(sqlFieldsQry);
}
/** Checks failed SqlFieldsQuery. */
@Test
- public void testSqlFieldsQueryFailed() {
+ public void testSqlFieldsQueryFailed() throws Exception {
runFailedQuery(sqlFieldsQryFailed);
}
/** Checks successful cross-cache SqlFieldsQuery. */
@Test
- public void testSqlFieldsCrossCacheQuery() {
+ public void testSqlFieldsCrossCacheQuery() throws Exception {
runSuccessfulQuery(sqlFieldsQryCrossCache);
}
/** Checks failed cross-cache SqlFieldsQuery. */
@Test
- public void testSqlFieldsCrossCacheQueryFailed() {
+ public void testSqlFieldsCrossCacheQueryFailed() throws Exception {
runFailedQuery(sqlFieldsQryCrossCacheFailed);
}
- /** Checks successful SqlFieldsQuery with reduce phase. */
+ /**
+ * Checks successful SqlFieldsQuery with reduce phase. There should be 3
entries in SQL plan history on the
+ * reduce node (2 SELECT subqueries and 1 UNION/MERGE subquery) and 2
entries on the map node (2 SELECT subqueries).
+ */
@Test
- public void testSqlFieldsQueryWithReducePhase() {
+ public void testSqlFieldsQueryWithReducePhase() throws Exception {
runQueryWithReducePhase(() -> {
- cacheQuery(sqlFieldsQryWithReducePhase.setLocal(loc), "pers");
+ try {
+ startGridsMultiThreaded(1, 2);
+
+ awaitPartitionMapExchange();
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ cacheQuery(sqlFieldsQryWithReducePhase, "pers");
+
+ checkSqlPlanHistory(3);
- checkSqlPlanHistory((!isClient && sqlEngine ==
IndexingQueryEngineConfiguration.ENGINE_NAME) ? 3 : 1);
+ for (int i = 1; i <= 2; i++) {
+ Map<SqlPlan, Long> sqlPlansOnMapNode =
grid(i).context().query().runningQueryManager()
Review Comment:
Use view, not direct access to tracker.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java:
##########
@@ -291,141 +288,168 @@ public void testJdbcQueryFailed() throws Exception {
/** Checks successful SqlFieldsQuery. */
@Test
- public void testSqlFieldsQuery() {
+ public void testSqlFieldsQuery() throws Exception {
runSuccessfulQuery(sqlFieldsQry);
}
/** Checks failed SqlFieldsQuery. */
@Test
- public void testSqlFieldsQueryFailed() {
+ public void testSqlFieldsQueryFailed() throws Exception {
runFailedQuery(sqlFieldsQryFailed);
}
/** Checks successful cross-cache SqlFieldsQuery. */
@Test
- public void testSqlFieldsCrossCacheQuery() {
+ public void testSqlFieldsCrossCacheQuery() throws Exception {
runSuccessfulQuery(sqlFieldsQryCrossCache);
}
/** Checks failed cross-cache SqlFieldsQuery. */
@Test
- public void testSqlFieldsCrossCacheQueryFailed() {
+ public void testSqlFieldsCrossCacheQueryFailed() throws Exception {
runFailedQuery(sqlFieldsQryCrossCacheFailed);
}
- /** Checks successful SqlFieldsQuery with reduce phase. */
+ /**
+ * Checks successful SqlFieldsQuery with reduce phase. There should be 3
entries in SQL plan history on the
+ * reduce node (2 SELECT subqueries and 1 UNION/MERGE subquery) and 2
entries on the map node (2 SELECT subqueries).
+ */
@Test
- public void testSqlFieldsQueryWithReducePhase() {
+ public void testSqlFieldsQueryWithReducePhase() throws Exception {
runQueryWithReducePhase(() -> {
- cacheQuery(sqlFieldsQryWithReducePhase.setLocal(loc), "pers");
+ try {
+ startGridsMultiThreaded(1, 2);
+
+ awaitPartitionMapExchange();
+ }
+ catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ cacheQuery(sqlFieldsQryWithReducePhase, "pers");
+
+ checkSqlPlanHistory(3);
- checkSqlPlanHistory((!isClient && sqlEngine ==
IndexingQueryEngineConfiguration.ENGINE_NAME) ? 3 : 1);
+ for (int i = 1; i <= 2; i++) {
+ Map<SqlPlan, Long> sqlPlansOnMapNode =
grid(i).context().query().runningQueryManager()
+ .planHistoryTracker().sqlPlanHistory();
+
+ assertNotNull(sqlPlansOnMapNode);
+
+ checkMetrics(2, sqlPlansOnMapNode);
+ }
});
}
- /** Checks failed SqlFieldsQuery with reduce phase. */
+ /**
+ * Checks failed SqlFieldsQuery with reduce phase. If the fisrt subquery
fails, there should be only one entry in
Review Comment:
Typo: `fisrt`
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java:
##########
@@ -480,26 +527,26 @@ public void runFailedQuery(Query qry) {
/**
* @param qry Query.
*/
- public void runQueryWithoutPlan(Query qry) {
+ public void runQueryWithoutPlan(Query qry) throws Exception {
+ startTestGrid();
+
cacheQuery(qry.setLocal(loc), "A");
checkSqlPlanHistory(0);
}
/**
- * @param task Task to execute.
+ * @param task Test task to execute.
*/
- public void runQueryWithReducePhase(Runnable task) {
+ public void runQueryWithReducePhase(Runnable task) throws Exception {
+ assumeFalse("Map/reduce queries are only applicable to H2 engine",
Review Comment:
`assumeFalse(..., x != y)` -> `assumeTrue(..., x == y)` for better
readability
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SqlPlanHistoryIntegrationTest.java:
##########
@@ -572,19 +622,27 @@ public void runSqlFieldsDml(IgniteBiTuple<List<String>,
Boolean> qrysInfo) {
* @param qrysInfo DML commands info (queries, simple query flag).
* @param task Task to be executed.
*/
- public void executeDml(IgniteBiTuple<List<String>, Boolean> qrysInfo,
Consumer<List<String>> task) {
+ public void executeDml(IgniteBiTuple<List<String>, Boolean> qrysInfo,
Consumer<List<String>> task) throws Exception {
assumeFalse("There is no lazy mode for DML operations",
!isFullyFetched);
Review Comment:
`assumeFalse(..., !x)` -> `assumeTrue(..., x)` for better readability
--
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]
