[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2164: IGNITE-17765 Sql. Avoid parsing queries that already have cached plans.

[via GitHub]

AMashenkov commented on code in PR #2164:
URL: https://github.com/apache/ignite-3/pull/2164#discussion_r1226680971


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -407,70 +423,83 @@ private CompletableFuture>> 
querySingle0(
 
 CompletableFuture start = new CompletableFuture<>();
 
+boolean implicitTxRequired = outerTx == null;
 AtomicReference tx = new AtomicReference<>();
 
 CompletableFuture>> stage = start
-.thenApply(v -> {
-StatementParseResult parseResult = 
IgniteSqlParser.parse(sql, StatementParseResult.MODE);
-SqlNode sqlNode = parseResult.statement();
-
-validateParsedStatement(context, outerTx, parseResult, 
sqlNode, params);
-
-return sqlNode;
-})
-.thenCompose(sqlNode -> {
-boolean rwOp = dataModificationOp(sqlNode);
-
-boolean implicitTxRequired = outerTx == null;
-
-tx.set(implicitTxRequired ? txManager.begin(!rwOp) : 
outerTx);
-
-SchemaPlus schema = sqlSchemaManager.schema(schemaName);
-
-if (schema == null) {
-return CompletableFuture.failedFuture(new 
SchemaNotFoundException(schemaName));
-}
-
-BaseQueryContext ctx = BaseQueryContext.builder()
-.frameworkConfig(
-
Frameworks.newConfigBuilder(FRAMEWORK_CONFIG)
-.defaultSchema(schema)
-.build()
-)
+.thenCompose(v -> {
+Builder contextBuilder = BaseQueryContext.builder()
 .logger(LOG)
 .cancel(queryCancel)
 .parameters(params)
-.plannerTimeout(PLANNER_TIMEOUT)
-.build();
-
-return prepareSvc.prepareAsync(sqlNode, ctx)
-.thenApply(plan -> {
-var dataCursor = 
executionSrvc.executePlan(tx.get(), plan, ctx);
-
-SqlQueryType queryType = plan.type();
-assert queryType != null : "Expected a full 
plan but got a fragment: " + plan;
-
-return new AsyncSqlCursorImpl<>(
-queryType,
-plan.metadata(),
-implicitTxRequired ? tx.get() : null,
-new AsyncCursor>() {
-@Override
-public 
CompletableFuture>> requestNextAsync(int rows) {
-session.touch();
-
-return 
dataCursor.requestNextAsync(rows);
-}
-
-@Override
-public CompletableFuture 
closeAsync() {
-session.touch();
-
-return dataCursor.closeAsync();
-}
-}
-);
-});
+.plannerTimeout(PLANNER_TIMEOUT);
+
+CompletableFuture[] newPlanHolder = new 
CompletableFuture[1];
+
+CompletableFuture cachedPlan = 
queryCache.computeIfAbsent(new CacheKey(schemaName, sql, params), (k) -> {

Review Comment:
   The lamda that is passed to `queryCache.computeIfAbsent` is run inside the 
synchronized block. For queries, which shouldn't be cached (like DDL), this 
maybe overkill.
   
   `queryCache.computeIfAbsent` already returns the future. Thus, there is no 
need to do any long operation inside the synchronized block.
   
   Instead, you can always return a future from lamda for QUERY and DML with 
putting computation to another thread. For non-QUERY or non-DML just return 
null and do computation in the current thread.
   Or maybe avoid using compute at all for sake of code simplicity.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2164: IGNITE-17765 Sql. Avoid parsing queries that already have cached plans.

[via GitHub]

AMashenkov commented on code in PR #2164:
URL: https://github.com/apache/ignite-3/pull/2164#discussion_r1226702935


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -407,70 +423,83 @@ private CompletableFuture>> 
querySingle0(
 
 CompletableFuture start = new CompletableFuture<>();
 
+boolean implicitTxRequired = outerTx == null;
 AtomicReference tx = new AtomicReference<>();
 
 CompletableFuture>> stage = start
-.thenApply(v -> {
-StatementParseResult parseResult = 
IgniteSqlParser.parse(sql, StatementParseResult.MODE);
-SqlNode sqlNode = parseResult.statement();
-
-validateParsedStatement(context, outerTx, parseResult, 
sqlNode, params);
-
-return sqlNode;
-})
-.thenCompose(sqlNode -> {
-boolean rwOp = dataModificationOp(sqlNode);
-
-boolean implicitTxRequired = outerTx == null;
-
-tx.set(implicitTxRequired ? txManager.begin(!rwOp) : 
outerTx);
-
-SchemaPlus schema = sqlSchemaManager.schema(schemaName);
-
-if (schema == null) {
-return CompletableFuture.failedFuture(new 
SchemaNotFoundException(schemaName));
-}
-
-BaseQueryContext ctx = BaseQueryContext.builder()
-.frameworkConfig(
-
Frameworks.newConfigBuilder(FRAMEWORK_CONFIG)
-.defaultSchema(schema)
-.build()
-)
+.thenCompose(v -> {
+Builder contextBuilder = BaseQueryContext.builder()
 .logger(LOG)
 .cancel(queryCancel)
 .parameters(params)
-.plannerTimeout(PLANNER_TIMEOUT)
-.build();
-
-return prepareSvc.prepareAsync(sqlNode, ctx)
-.thenApply(plan -> {
-var dataCursor = 
executionSrvc.executePlan(tx.get(), plan, ctx);
-
-SqlQueryType queryType = plan.type();
-assert queryType != null : "Expected a full 
plan but got a fragment: " + plan;
-
-return new AsyncSqlCursorImpl<>(
-queryType,
-plan.metadata(),
-implicitTxRequired ? tx.get() : null,
-new AsyncCursor>() {
-@Override
-public 
CompletableFuture>> requestNextAsync(int rows) {
-session.touch();
-
-return 
dataCursor.requestNextAsync(rows);
-}
-
-@Override
-public CompletableFuture 
closeAsync() {
-session.touch();
-
-return dataCursor.closeAsync();
-}
-}
-);
-});
+.plannerTimeout(PLANNER_TIMEOUT);
+
+CompletableFuture[] newPlanHolder = new 
CompletableFuture[1];
+
+CompletableFuture cachedPlan = 
queryCache.computeIfAbsent(new CacheKey(schemaName, sql, params), (k) -> {
+// Parse query.
+StatementParseResult parseResult = 
IgniteSqlParser.parse(sql, StatementParseResult.MODE);
+SqlNode sqlNode = parseResult.statement();
+
+validateParsedStatement(context, outerTx, parseResult, 
sqlNode, params);
+
+// Build context.
+tx.set(implicitTxRequired ? 
txManager.begin(!dataModificationOp(sqlNode)) : outerTx);

Review Comment:
   Let's add an assert 
   `assert outerTx == null || !outerTx.readOnly()`



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2164: IGNITE-17765 Sql. Avoid parsing queries that already have cached plans.

[via GitHub]

AMashenkov commented on code in PR #2164:
URL: https://github.com/apache/ignite-3/pull/2164#discussion_r1226702935


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -407,70 +423,83 @@ private CompletableFuture>> 
querySingle0(
 
 CompletableFuture start = new CompletableFuture<>();
 
+boolean implicitTxRequired = outerTx == null;
 AtomicReference tx = new AtomicReference<>();
 
 CompletableFuture>> stage = start
-.thenApply(v -> {
-StatementParseResult parseResult = 
IgniteSqlParser.parse(sql, StatementParseResult.MODE);
-SqlNode sqlNode = parseResult.statement();
-
-validateParsedStatement(context, outerTx, parseResult, 
sqlNode, params);
-
-return sqlNode;
-})
-.thenCompose(sqlNode -> {
-boolean rwOp = dataModificationOp(sqlNode);
-
-boolean implicitTxRequired = outerTx == null;
-
-tx.set(implicitTxRequired ? txManager.begin(!rwOp) : 
outerTx);
-
-SchemaPlus schema = sqlSchemaManager.schema(schemaName);
-
-if (schema == null) {
-return CompletableFuture.failedFuture(new 
SchemaNotFoundException(schemaName));
-}
-
-BaseQueryContext ctx = BaseQueryContext.builder()
-.frameworkConfig(
-
Frameworks.newConfigBuilder(FRAMEWORK_CONFIG)
-.defaultSchema(schema)
-.build()
-)
+.thenCompose(v -> {
+Builder contextBuilder = BaseQueryContext.builder()
 .logger(LOG)
 .cancel(queryCancel)
 .parameters(params)
-.plannerTimeout(PLANNER_TIMEOUT)
-.build();
-
-return prepareSvc.prepareAsync(sqlNode, ctx)
-.thenApply(plan -> {
-var dataCursor = 
executionSrvc.executePlan(tx.get(), plan, ctx);
-
-SqlQueryType queryType = plan.type();
-assert queryType != null : "Expected a full 
plan but got a fragment: " + plan;
-
-return new AsyncSqlCursorImpl<>(
-queryType,
-plan.metadata(),
-implicitTxRequired ? tx.get() : null,
-new AsyncCursor>() {
-@Override
-public 
CompletableFuture>> requestNextAsync(int rows) {
-session.touch();
-
-return 
dataCursor.requestNextAsync(rows);
-}
-
-@Override
-public CompletableFuture 
closeAsync() {
-session.touch();
-
-return dataCursor.closeAsync();
-}
-}
-);
-});
+.plannerTimeout(PLANNER_TIMEOUT);
+
+CompletableFuture[] newPlanHolder = new 
CompletableFuture[1];
+
+CompletableFuture cachedPlan = 
queryCache.computeIfAbsent(new CacheKey(schemaName, sql, params), (k) -> {
+// Parse query.
+StatementParseResult parseResult = 
IgniteSqlParser.parse(sql, StatementParseResult.MODE);
+SqlNode sqlNode = parseResult.statement();
+
+validateParsedStatement(context, outerTx, parseResult, 
sqlNode, params);
+
+// Build context.
+tx.set(implicitTxRequired ? 
txManager.begin(!dataModificationOp(sqlNode)) : outerTx);

Review Comment:
   Let's add an assert 
   `assert outerTx == null || outerTx.readOnly() != dataModificationOp(sqlNode)`



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2164: IGNITE-17765 Sql. Avoid parsing queries that already have cached plans.

[via GitHub]

AMashenkov commented on code in PR #2164:
URL: https://github.com/apache/ignite-3/pull/2164#discussion_r1226710781


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/CacheKey.java:
##
@@ -18,48 +18,34 @@
 package org.apache.ignite.internal.sql.engine.prepare;
 
 import java.util.Arrays;
-import java.util.Objects;
 
 /**
  * CacheKey.
  * The class uses to distinguish different query plans which could be various 
for the same query text but different context. As example such
  * context could be schema name, dynamic parameters, and so on...
  */
 public class CacheKey {
-public static final Class[] EMPTY_CLASS_ARRAY = {};
+private static final Class[] EMPTY_CLASS_ARRAY = {};
 
 private final String schemaName;
 
 private final String query;
 
-private final Object contextKey;
-
 private final Class[] paramTypes;
 
 /**
  * Constructor.
  *
  * @param schemaName Schema name.
  * @param query  Query string.
- * @param contextKey Optional context key to differ queries with and 
without/different flags, having an impact on result plan (like
- *   LOCAL flag)
- * @param paramTypes Types of all dynamic parameters, no any type can be 
{@code null}.
+ * @param params Dynamic parameters.
  */
-public CacheKey(String schemaName, String query, Object contextKey, 
Class[] paramTypes) {
+public CacheKey(String schemaName, String query, Object[] params) {

Review Comment:
   Cache key must contains schema version.
   Using integer CatalogSchemaDescriptor.id() instead of schemaName would be 
more optimal.
   
   As of now, you can safely hardcode constants instead of version/id, which we 
will fix later.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2164: IGNITE-17765 Sql. Avoid parsing queries that already have cached plans.

[via GitHub]

AMashenkov commented on code in PR #2164:
URL: https://github.com/apache/ignite-3/pull/2164#discussion_r1226710781


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/CacheKey.java:
##
@@ -18,48 +18,34 @@
 package org.apache.ignite.internal.sql.engine.prepare;
 
 import java.util.Arrays;
-import java.util.Objects;
 
 /**
  * CacheKey.
  * The class uses to distinguish different query plans which could be various 
for the same query text but different context. As example such
  * context could be schema name, dynamic parameters, and so on...
  */
 public class CacheKey {
-public static final Class[] EMPTY_CLASS_ARRAY = {};
+private static final Class[] EMPTY_CLASS_ARRAY = {};
 
 private final String schemaName;
 
 private final String query;
 
-private final Object contextKey;
-
 private final Class[] paramTypes;
 
 /**
  * Constructor.
  *
  * @param schemaName Schema name.
  * @param query  Query string.
- * @param contextKey Optional context key to differ queries with and 
without/different flags, having an impact on result plan (like
- *   LOCAL flag)
- * @param paramTypes Types of all dynamic parameters, no any type can be 
{@code null}.
+ * @param params Dynamic parameters.
  */
-public CacheKey(String schemaName, String query, Object contextKey, 
Class[] paramTypes) {
+public CacheKey(String schemaName, String query, Object[] params) {

Review Comment:
   Cache key must contains schema version.
   Using integer CatalogSchemaDescriptor.id() instead of schemaName would be 
more optimal.
   
   As of now, you can safely hardcode constants instead of version/id, which we 
will fix later, when SqlManager wil be switched to Catalog.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2164: IGNITE-17765 Sql. Avoid parsing queries that already have cached plans.

[via GitHub]

AMashenkov commented on code in PR #2164:
URL: https://github.com/apache/ignite-3/pull/2164#discussion_r1226729461


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/CacheKey.java:
##
@@ -80,9 +66,6 @@ public boolean equals(Object o) {
 if (!query.equals(cacheKey.query)) {
 return false;
 }
-if (!Objects.equals(contextKey, cacheKey.contextKey)) {
-return false;
-}
 
 return Arrays.deepEquals(paramTypes, cacheKey.paramTypes);

Review Comment:
   Arrays.equals/hashCode can be used instead of deepEquals/deepHashCode.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2164: IGNITE-17765 Sql. Avoid parsing queries that already have cached plans.

[via GitHub]

AMashenkov commented on code in PR #2164:
URL: https://github.com/apache/ignite-3/pull/2164#discussion_r1226747351


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -244,7 +260,7 @@ public synchronized void start() {
 busyLock
 );
 
-sqlSchemaManager.registerListener(prepareSvc);
+sqlSchemaManager.registerListener(queryCache::clear);

Review Comment:
   With versioned schema, there is no need to clear cache. Plans for the old 
schema versions could be used by time-traveling queries.
   Let's avoid this, or add a TODO to drop this line later.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] AMashenkov merged pull request #2174: IGNITE-19304 Forbid creating index on the duplicate columns

[via GitHub]

AMashenkov merged PR #2174:
URL: https://github.com/apache/ignite-3/pull/2174


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2140: [IGNITE-19587] Sql. Remove execution-related part from IgniteTable.

[via GitHub]

AMashenkov commented on code in PR #2140:
URL: https://github.com/apache/ignite-3/pull/2140#discussion_r1226776026


##
modules/core/src/main/java/org/apache/ignite/internal/thread/StripedThreadPoolExecutor.java:
##
@@ -105,7 +106,7 @@ public void execute(Runnable cmd) {
  * @throws NullPointerException   if the task is {@code null}.
  */
 public CompletableFuture submit(Runnable task, int idx) {
-return CompletableFuture.runAsync(task, execs[threadId(idx)]);
+return CompletableFuture.runAsync(task, commandExecutor(idx));

Review Comment:
   `@param idx` is missed in javadoc.
   Would you please fix 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.

To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] lowka commented on a diff in pull request #2140: [IGNITE-19587] Sql. Remove execution-related part from IgniteTable.

[via GitHub]

lowka commented on code in PR #2140:
URL: https://github.com/apache/ignite-3/pull/2140#discussion_r1226780794


##
modules/core/src/main/java/org/apache/ignite/internal/thread/StripedThreadPoolExecutor.java:
##
@@ -105,7 +106,7 @@ public void execute(Runnable cmd) {
  * @throws NullPointerException   if the task is {@code null}.
  */
 public CompletableFuture submit(Runnable task, int idx) {
-return CompletableFuture.runAsync(task, execs[threadId(idx)]);
+return CompletableFuture.runAsync(task, commandExecutor(idx));

Review Comment:
   Done.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] xtern commented on a diff in pull request #2164: IGNITE-17765 Sql. Avoid parsing queries that already have cached plans.

[via GitHub]

xtern commented on code in PR #2164:
URL: https://github.com/apache/ignite-3/pull/2164#discussion_r1227149541


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -407,70 +423,83 @@ private CompletableFuture>> 
querySingle0(
 
 CompletableFuture start = new CompletableFuture<>();
 
+boolean implicitTxRequired = outerTx == null;
 AtomicReference tx = new AtomicReference<>();
 
 CompletableFuture>> stage = start
-.thenApply(v -> {
-StatementParseResult parseResult = 
IgniteSqlParser.parse(sql, StatementParseResult.MODE);
-SqlNode sqlNode = parseResult.statement();
-
-validateParsedStatement(context, outerTx, parseResult, 
sqlNode, params);
-
-return sqlNode;
-})
-.thenCompose(sqlNode -> {
-boolean rwOp = dataModificationOp(sqlNode);
-
-boolean implicitTxRequired = outerTx == null;
-
-tx.set(implicitTxRequired ? txManager.begin(!rwOp) : 
outerTx);
-
-SchemaPlus schema = sqlSchemaManager.schema(schemaName);
-
-if (schema == null) {
-return CompletableFuture.failedFuture(new 
SchemaNotFoundException(schemaName));
-}
-
-BaseQueryContext ctx = BaseQueryContext.builder()
-.frameworkConfig(
-
Frameworks.newConfigBuilder(FRAMEWORK_CONFIG)
-.defaultSchema(schema)
-.build()
-)
+.thenCompose(v -> {
+Builder contextBuilder = BaseQueryContext.builder()
 .logger(LOG)
 .cancel(queryCancel)
 .parameters(params)
-.plannerTimeout(PLANNER_TIMEOUT)
-.build();
-
-return prepareSvc.prepareAsync(sqlNode, ctx)
-.thenApply(plan -> {
-var dataCursor = 
executionSrvc.executePlan(tx.get(), plan, ctx);
-
-SqlQueryType queryType = plan.type();
-assert queryType != null : "Expected a full 
plan but got a fragment: " + plan;
-
-return new AsyncSqlCursorImpl<>(
-queryType,
-plan.metadata(),
-implicitTxRequired ? tx.get() : null,
-new AsyncCursor>() {
-@Override
-public 
CompletableFuture>> requestNextAsync(int rows) {
-session.touch();
-
-return 
dataCursor.requestNextAsync(rows);
-}
-
-@Override
-public CompletableFuture 
closeAsync() {
-session.touch();
-
-return dataCursor.closeAsync();
-}
-}
-);
-});
+.plannerTimeout(PLANNER_TIMEOUT);
+
+CompletableFuture[] newPlanHolder = new 
CompletableFuture[1];
+
+CompletableFuture cachedPlan = 
queryCache.computeIfAbsent(new CacheKey(schemaName, sql, params), (k) -> {

Review Comment:
   > Instead, you can always return a future from lamda for QUERY and DML with 
putting computation to another thread.
   
   This is the main problem - I can't do this, because we need to parse the 
query to understand the type of query 😔 .



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-extensions] Noone-No commented on pull request #94: IGNITE-16124 change signature in method deleteAllById in spring data 2.2

[via GitHub]

Noone-No commented on PR #94:
URL: https://github.com/apache/ignite-extensions/pull/94#issuecomment-1588492617

   how to fix this error ? 


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] ptupitsyn merged pull request #2180: IGNITE-19707 Fix AssertionError in DistributionZoneManagerScaleUpTest

[via GitHub]

ptupitsyn merged PR #2180:
URL: https://github.com/apache/ignite-3/pull/2180


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] alievmirza commented on a diff in pull request #2171: IGNITE-19600 Removed topologyVersionedDataNodes

[via GitHub]

alievmirza commented on code in PR #2171:
URL: https://github.com/apache/ignite-3/pull/2171#discussion_r1227592992


##
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerLogicalTopologyEventsTest.java:
##
@@ -45,6 +48,9 @@ void testMetaStorageKeysInitializedOnStartWhenTopVerEmpty() 
throws Exception {
 
 distributionZoneManager.start();
 
+verify(keyValueStorage, timeout(1000).times(1))

Review Comment:
   why do we need changed in this class?



##
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerLogicalTopologyEventsTest.java:
##
@@ -45,6 +48,9 @@ void testMetaStorageKeysInitializedOnStartWhenTopVerEmpty() 
throws Exception {
 
 distributionZoneManager.start();
 
+verify(keyValueStorage, timeout(1000).times(1))

Review Comment:
   why do we need changes in this class?



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] vldpyatkov opened a new pull request, #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

vldpyatkov opened a new pull request, #2183:
URL: https://github.com/apache/ignite-3/pull/2183

   …geManager.start()


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] timoninmaxim commented on a diff in pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

timoninmaxim commented on code in PR #10767:
URL: https://github.com/apache/ignite/pull/10767#discussion_r1227630007


##
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryLimitTest.java:
##
@@ -0,0 +1,249 @@
+/*
+ * 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.cache.query;
+
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.IntUnaryOperator;
+import javax.cache.Cache;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.cache.query.annotations.QuerySqlField;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.processors.cache.query.QueryCursorEx;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheMode.REPLICATED;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.lt;
+
+/** */
+public class IndexQueryLimitTest extends GridCommonAbstractTest {
+/** */
+private static final String CACHE = "TEST_CACHE";
+
+/** */
+private static final String IDX = "PERSON_ID_IDX";
+
+/** */
+private static final int CNT = 10_000;
+
+/** */
+private Ignite crd;
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+crd = startGrids(4);
+}
+
+/** {@inheritDoc} */
+@Override protected void afterTest() {
+stopAllGrids();
+}
+
+/** {@inheritDoc} */
+@Override protected IgniteConfiguration getConfiguration(String 
igniteInstanceName) throws Exception {
+IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+CacheConfiguration ccfg = new CacheConfiguration()
+.setName(CACHE)
+.setIndexedTypes(Long.class, Person.class)
+.setAtomicityMode(TRANSACTIONAL)
+.setCacheMode(REPLICATED)
+.setQueryParallelism(1)
+.setBackups(0);

Review Comment:
   No need to set, backups=0 is default value.



##
modules/core/src/main/java/org/apache/ignite/internal/processors/platform/client/cache/ClientCacheIndexQueryRequest.java:
##
@@ -47,8 +49,10 @@ public class ClientCacheIndexQueryRequest extends 
ClientCacheQueryRequest {
 
 /**
  * @param reader Reader.
+ * @param protocolCtx
  */
-public ClientCacheIndexQueryRequest(BinaryRawReaderEx reader) {
+public ClientCacheIndexQueryRequest(BinaryRawReaderEx reader,

Review Comment:
   ```suggestion
   public ClientCacheIndexQueryRequest(
   BinaryRawReaderEx reader,
   ClientProtocolContext protocolCtx
   ) {
   ```



##
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryLimitTest.java:
##
@@ -0,0 +1,249 @@
+/*
+ * 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.cache.query;
+
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Rand

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2140: [IGNITE-19587] Sql. Remove execution-related part from IgniteTable.

[via GitHub]

korlov42 commented on code in PR #2140:
URL: https://github.com/apache/ignite-3/pull/2140#discussion_r1226605360


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/QueryTaskExecutor.java:
##
@@ -45,4 +45,13 @@ public interface QueryTaskExecutor extends LifecycleAware, 
Executor {
  * @return the new CompletableFuture
  */
 CompletableFuture submit(UUID qryId, long fragmentId, Runnable qryTask);
+
+/**
+ * Returns an executor that is responsible for the given query fragment.
+ *
+ * @param qryId  Query ID.
+ * @param fragmentId Fragment ID.
+ * @return  An executor.
+ */
+Executor fragmentExecutor(UUID qryId, long fragmentId);

Review Comment:
   Sole purpose of query task executer is to hide presence of several 
executers, while providing necessary guarantee for execution order of tasks 
from the same fragment.



##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutableTable.java:
##
@@ -0,0 +1,41 @@
+/*
+ * 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.sql.engine.exec;
+
+import org.apache.ignite.internal.table.InternalTable;
+
+/**
+ * Execution related APIs of a table.
+ */
+public interface ExecutableTable {
+
+/**
+ * Returns read API.
+ */
+InternalTable table();

Review Comment:
   It's better to introduce something like ScannableTable (similar to 
UpdateableTable).
   
   BTW, let's rename UpdateableTable --> UpdatableTable to make naming more 
consistent 



##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -114,6 +116,9 @@ public class SqlQueryProcessor implements QueryProcessor {
 /** Size of the cache for query plans. */
 private static final int PLAN_CACHE_SIZE = 1024;
 
+/** Size of the table access cache. */
+public static final int TABLE_CACHE_SIZE = 1024;

Review Comment:
   why `public`?



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] lowka commented on a diff in pull request #2140: [IGNITE-19587] Sql. Remove execution-related part from IgniteTable.

[via GitHub]

lowka commented on code in PR #2140:
URL: https://github.com/apache/ignite-3/pull/2140#discussion_r1227689796


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutableTable.java:
##
@@ -0,0 +1,41 @@
+/*
+ * 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.sql.engine.exec;
+
+import org.apache.ignite.internal.table.InternalTable;
+
+/**
+ * Execution related APIs of a table.
+ */
+public interface ExecutableTable {
+
+/**
+ * Returns read API.
+ */
+InternalTable table();

Review Comment:
   will do.
   



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] denis-chudov closed pull request #2115: IGNITE-19478 Possible excess size of table entities written into meta storage

[via GitHub]

denis-chudov closed pull request #2115: IGNITE-19478 Possible excess size of 
table entities written into meta storage
URL: https://github.com/apache/ignite-3/pull/2115


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] denis-chudov closed pull request #1875: placement driver start

[via GitHub]

denis-chudov closed pull request #1875: placement driver start
URL: https://github.com/apache/ignite-3/pull/1875


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] denis-chudov closed pull request #2014: wip

[via GitHub]

denis-chudov closed pull request #2014: wip
URL: https://github.com/apache/ignite-3/pull/2014


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] ivandasch commented on a diff in pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

ivandasch commented on code in PR #10675:
URL: https://github.com/apache/ignite/pull/10675#discussion_r1227780600


##
modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/ArgumentParser.java:
##
@@ -0,0 +1,514 @@
+/*
+ * 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.commandline;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.IgniteSystemProperties;
+import org.apache.ignite.internal.client.GridClientConfiguration;
+import org.apache.ignite.internal.commandline.argument.parser.CLIArgument;
+import 
org.apache.ignite.internal.commandline.argument.parser.CLIArgumentParser;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.IgniteCommandRegistry;
+import org.apache.ignite.internal.management.api.Argument;
+import org.apache.ignite.internal.management.api.ArgumentGroup;
+import org.apache.ignite.internal.management.api.BeforeNodeStartCommand;
+import org.apache.ignite.internal.management.api.CliSubcommandsWithPrefix;
+import org.apache.ignite.internal.management.api.Command;
+import org.apache.ignite.internal.management.api.CommandsRegistry;
+import org.apache.ignite.internal.management.api.ComputeCommand;
+import org.apache.ignite.internal.management.api.HelpCommand;
+import org.apache.ignite.internal.management.api.LocalCommand;
+import org.apache.ignite.internal.management.api.Positional;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.apache.ignite.lang.IgniteExperimental;
+import org.apache.ignite.ssl.SslContextFactory;
+
+import static 
org.apache.ignite.IgniteSystemProperties.IGNITE_ENABLE_EXPERIMENTAL_COMMAND;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_INTERVAL;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_TIMEOUT;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_HOST;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_PORT;
+import static 
org.apache.ignite.internal.commandline.CommandHandler.UTILITY_NAME;
+import static 
org.apache.ignite.internal.commandline.argument.parser.CLIArgument.optionalArg;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.CMD_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.NAME_PREFIX;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.PARAM_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.asOptional;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.fromFormattedCommandName;
+import static org.apache.ignite.internal.management.api.CommandUtils.isBoolean;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.parameterExample;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedCommandName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedFieldName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedNames;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.visitCommandParams;
+import static org.apache.ignite.ssl.SslContextFactory.DFLT_SSL_PROTOCOL;
+
+/**
+ * Argument parser.
+ * Also would parse high-level command and delegate parsing for its argument 
to the command.
+ */
+public class ArgumentParser {
+/** */
+private final IgniteLogger log;
+
+/** */
+private final IgniteCommandRegistry registry;
+
+/** */
+static final String CMD_HOST = "--host";
+
+/** */
+static final St

[GitHub] [ignite] ivandasch commented on a diff in pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

ivandasch commented on code in PR #10675:
URL: https://github.com/apache/ignite/pull/10675#discussion_r1227782321


##
modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/ArgumentParser.java:
##
@@ -0,0 +1,514 @@
+/*
+ * 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.commandline;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.IgniteSystemProperties;
+import org.apache.ignite.internal.client.GridClientConfiguration;
+import org.apache.ignite.internal.commandline.argument.parser.CLIArgument;
+import 
org.apache.ignite.internal.commandline.argument.parser.CLIArgumentParser;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.IgniteCommandRegistry;
+import org.apache.ignite.internal.management.api.Argument;
+import org.apache.ignite.internal.management.api.ArgumentGroup;
+import org.apache.ignite.internal.management.api.BeforeNodeStartCommand;
+import org.apache.ignite.internal.management.api.CliSubcommandsWithPrefix;
+import org.apache.ignite.internal.management.api.Command;
+import org.apache.ignite.internal.management.api.CommandsRegistry;
+import org.apache.ignite.internal.management.api.ComputeCommand;
+import org.apache.ignite.internal.management.api.HelpCommand;
+import org.apache.ignite.internal.management.api.LocalCommand;
+import org.apache.ignite.internal.management.api.Positional;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.apache.ignite.lang.IgniteExperimental;
+import org.apache.ignite.ssl.SslContextFactory;
+
+import static 
org.apache.ignite.IgniteSystemProperties.IGNITE_ENABLE_EXPERIMENTAL_COMMAND;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_INTERVAL;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_TIMEOUT;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_HOST;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_PORT;
+import static 
org.apache.ignite.internal.commandline.CommandHandler.UTILITY_NAME;
+import static 
org.apache.ignite.internal.commandline.argument.parser.CLIArgument.optionalArg;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.CMD_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.NAME_PREFIX;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.PARAM_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.asOptional;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.fromFormattedCommandName;
+import static org.apache.ignite.internal.management.api.CommandUtils.isBoolean;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.parameterExample;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedCommandName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedFieldName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedNames;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.visitCommandParams;
+import static org.apache.ignite.ssl.SslContextFactory.DFLT_SSL_PROTOCOL;
+
+/**
+ * Argument parser.
+ * Also would parse high-level command and delegate parsing for its argument 
to the command.
+ */
+public class ArgumentParser {
+/** */
+private final IgniteLogger log;
+
+/** */
+private final IgniteCommandRegistry registry;
+
+/** */
+static final String CMD_HOST = "--host";
+
+/** */
+static final St

[GitHub] [ignite] ivandasch commented on a diff in pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

ivandasch commented on code in PR #10675:
URL: https://github.com/apache/ignite/pull/10675#discussion_r1227788649


##
modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/ArgumentParser.java:
##
@@ -0,0 +1,514 @@
+/*
+ * 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.commandline;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.IgniteSystemProperties;
+import org.apache.ignite.internal.client.GridClientConfiguration;
+import org.apache.ignite.internal.commandline.argument.parser.CLIArgument;
+import 
org.apache.ignite.internal.commandline.argument.parser.CLIArgumentParser;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.IgniteCommandRegistry;
+import org.apache.ignite.internal.management.api.Argument;
+import org.apache.ignite.internal.management.api.ArgumentGroup;
+import org.apache.ignite.internal.management.api.BeforeNodeStartCommand;
+import org.apache.ignite.internal.management.api.CliSubcommandsWithPrefix;
+import org.apache.ignite.internal.management.api.Command;
+import org.apache.ignite.internal.management.api.CommandsRegistry;
+import org.apache.ignite.internal.management.api.ComputeCommand;
+import org.apache.ignite.internal.management.api.HelpCommand;
+import org.apache.ignite.internal.management.api.LocalCommand;
+import org.apache.ignite.internal.management.api.Positional;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.apache.ignite.lang.IgniteExperimental;
+import org.apache.ignite.ssl.SslContextFactory;
+
+import static 
org.apache.ignite.IgniteSystemProperties.IGNITE_ENABLE_EXPERIMENTAL_COMMAND;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_INTERVAL;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_TIMEOUT;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_HOST;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_PORT;
+import static 
org.apache.ignite.internal.commandline.CommandHandler.UTILITY_NAME;
+import static 
org.apache.ignite.internal.commandline.argument.parser.CLIArgument.optionalArg;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.CMD_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.NAME_PREFIX;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.PARAM_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.asOptional;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.fromFormattedCommandName;
+import static org.apache.ignite.internal.management.api.CommandUtils.isBoolean;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.parameterExample;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedCommandName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedFieldName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedNames;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.visitCommandParams;
+import static org.apache.ignite.ssl.SslContextFactory.DFLT_SSL_PROTOCOL;
+
+/**
+ * Argument parser.
+ * Also would parse high-level command and delegate parsing for its argument 
to the command.
+ */
+public class ArgumentParser {
+/** */
+private final IgniteLogger log;
+
+/** */
+private final IgniteCommandRegistry registry;
+
+/** */
+static final String CMD_HOST = "--host";
+
+/** */
+static final St

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

sonarcloud[bot] commented on PR #10767:
URL: https://github.com/apache/ignite/pull/10767#issuecomment-1588864646

   Kudos, SonarCloud Quality Gate passed!    [![Quality Gate 
passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png
 'Quality Gate 
passed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10767)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
 [0 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
 [1 Code 
Smell](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_coverage&view=list)
  
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_duplicated_lines_density&view=list)
 [0.0% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] ivandasch commented on a diff in pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

ivandasch commented on code in PR #10675:
URL: https://github.com/apache/ignite/pull/10675#discussion_r1227806712


##
modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/CommandInvoker.java:
##
@@ -0,0 +1,316 @@
+/*
+ * 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.commandline;
+
+import java.io.IOException;
+import java.net.InetAddress;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.internal.client.GridClient;
+import org.apache.ignite.internal.client.GridClientBeforeNodeStart;
+import org.apache.ignite.internal.client.GridClientCompute;
+import org.apache.ignite.internal.client.GridClientConfiguration;
+import org.apache.ignite.internal.client.GridClientDisconnectedException;
+import org.apache.ignite.internal.client.GridClientException;
+import org.apache.ignite.internal.client.GridClientFactory;
+import org.apache.ignite.internal.client.GridClientNode;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.api.BeforeNodeStartCommand;
+import org.apache.ignite.internal.management.api.Command;
+import org.apache.ignite.internal.management.api.ComputeCommand;
+import org.apache.ignite.internal.management.api.LocalCommand;
+import org.apache.ignite.internal.management.api.PreparableCommand;
+import org.apache.ignite.internal.util.IgniteUtils;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.visor.VisorTaskArgument;
+import org.apache.ignite.lang.IgniteBiTuple;
+
+import static java.util.Collections.singleton;
+import static java.util.stream.Collectors.toMap;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_HOST;
+
+/**
+ * Adapter of new management API command for legacy {@code control.sh} 
execution flow.
+ */
+public class CommandInvoker {
+/** Command to execute. */
+private final Command cmd;
+
+/** Parsed argument. */
+private final A arg;
+
+/** Client configuration. */
+private GridClientConfiguration clientCfg;
+
+/** @param cmd Command to execute. */
+public CommandInvoker(Command cmd, A arg, GridClientConfiguration 
clientCfg) {
+this.cmd = cmd;
+this.arg = arg;
+this.clientCfg = clientCfg;
+}
+
+/**
+ * Actual command execution with verbose mode if needed.
+ * Implement it if your command supports verbose mode.
+ *
+ * @param log Logger to use.
+ * @param verbose Use verbose mode or not
+ * @return Result of operation (mostly usable for tests).
+ * @throws Exception If error occur.
+ */
+public  R invoke(IgniteLogger log, boolean verbose) throws Exception {
+try (GridClient client = startClient(clientCfg)) {
+String deprecationMsg = cmd.deprecationMessage(arg);
+
+if (deprecationMsg != null)
+log.warning(deprecationMsg);
+
+R res;
+
+if (cmd instanceof LocalCommand)
+res = ((LocalCommand)cmd).execute(client, arg, 
log::info);
+else if (cmd instanceof ComputeCommand) {
+GridClientCompute compute = client.compute();
+
+Map nodes = compute.nodes().stream()
+.collect(toMap(GridClientNode::nodeId, n -> n));
+
+ComputeCommand cmd = (ComputeCommand)this.cmd;
+
+Collection cmdNodes = cmd.nodes(nodes, arg);
+
+if (cmdNodes == null)
+cmdNodes = singleton(defaultNode(client, 
clientCfg).nodeId());
+
+for (UUID id : cmdNodes) {
+if (!nodes.containsKey(id))
+throw new IllegalArgumentException("Node with id=" + 
id + " not found.");
+}
+
+Collection connectable = F.viewReadOnly(
+cmdNodes,
+nodes::get,
+id -> nodes.get(id).connectable()
+);
+
+if (!F.isEmpty(connectable))
+ 

[GitHub] [ignite] nizhikov commented on a diff in pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

nizhikov commented on code in PR #10675:
URL: https://github.com/apache/ignite/pull/10675#discussion_r1227814904


##
modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/ArgumentParser.java:
##
@@ -0,0 +1,514 @@
+/*
+ * 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.commandline;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.IgniteSystemProperties;
+import org.apache.ignite.internal.client.GridClientConfiguration;
+import org.apache.ignite.internal.commandline.argument.parser.CLIArgument;
+import 
org.apache.ignite.internal.commandline.argument.parser.CLIArgumentParser;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.IgniteCommandRegistry;
+import org.apache.ignite.internal.management.api.Argument;
+import org.apache.ignite.internal.management.api.ArgumentGroup;
+import org.apache.ignite.internal.management.api.BeforeNodeStartCommand;
+import org.apache.ignite.internal.management.api.CliSubcommandsWithPrefix;
+import org.apache.ignite.internal.management.api.Command;
+import org.apache.ignite.internal.management.api.CommandsRegistry;
+import org.apache.ignite.internal.management.api.ComputeCommand;
+import org.apache.ignite.internal.management.api.HelpCommand;
+import org.apache.ignite.internal.management.api.LocalCommand;
+import org.apache.ignite.internal.management.api.Positional;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.apache.ignite.lang.IgniteExperimental;
+import org.apache.ignite.ssl.SslContextFactory;
+
+import static 
org.apache.ignite.IgniteSystemProperties.IGNITE_ENABLE_EXPERIMENTAL_COMMAND;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_INTERVAL;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_TIMEOUT;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_HOST;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_PORT;
+import static 
org.apache.ignite.internal.commandline.CommandHandler.UTILITY_NAME;
+import static 
org.apache.ignite.internal.commandline.argument.parser.CLIArgument.optionalArg;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.CMD_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.NAME_PREFIX;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.PARAM_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.asOptional;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.fromFormattedCommandName;
+import static org.apache.ignite.internal.management.api.CommandUtils.isBoolean;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.parameterExample;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedCommandName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedFieldName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedNames;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.visitCommandParams;
+import static org.apache.ignite.ssl.SslContextFactory.DFLT_SSL_PROTOCOL;
+
+/**
+ * Argument parser.
+ * Also would parse high-level command and delegate parsing for its argument 
to the command.
+ */
+public class ArgumentParser {
+/** */
+private final IgniteLogger log;
+
+/** */
+private final IgniteCommandRegistry registry;
+
+/** */
+static final String CMD_HOST = "--host";
+
+/** */
+static final Str

[GitHub] [ignite] nizhikov commented on a diff in pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

nizhikov commented on code in PR #10675:
URL: https://github.com/apache/ignite/pull/10675#discussion_r1227816389


##
modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/ArgumentParser.java:
##
@@ -0,0 +1,514 @@
+/*
+ * 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.commandline;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.IgniteSystemProperties;
+import org.apache.ignite.internal.client.GridClientConfiguration;
+import org.apache.ignite.internal.commandline.argument.parser.CLIArgument;
+import 
org.apache.ignite.internal.commandline.argument.parser.CLIArgumentParser;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.IgniteCommandRegistry;
+import org.apache.ignite.internal.management.api.Argument;
+import org.apache.ignite.internal.management.api.ArgumentGroup;
+import org.apache.ignite.internal.management.api.BeforeNodeStartCommand;
+import org.apache.ignite.internal.management.api.CliSubcommandsWithPrefix;
+import org.apache.ignite.internal.management.api.Command;
+import org.apache.ignite.internal.management.api.CommandsRegistry;
+import org.apache.ignite.internal.management.api.ComputeCommand;
+import org.apache.ignite.internal.management.api.HelpCommand;
+import org.apache.ignite.internal.management.api.LocalCommand;
+import org.apache.ignite.internal.management.api.Positional;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.apache.ignite.lang.IgniteExperimental;
+import org.apache.ignite.ssl.SslContextFactory;
+
+import static 
org.apache.ignite.IgniteSystemProperties.IGNITE_ENABLE_EXPERIMENTAL_COMMAND;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_INTERVAL;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_TIMEOUT;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_HOST;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_PORT;
+import static 
org.apache.ignite.internal.commandline.CommandHandler.UTILITY_NAME;
+import static 
org.apache.ignite.internal.commandline.argument.parser.CLIArgument.optionalArg;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.CMD_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.NAME_PREFIX;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.PARAM_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.asOptional;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.fromFormattedCommandName;
+import static org.apache.ignite.internal.management.api.CommandUtils.isBoolean;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.parameterExample;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedCommandName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedFieldName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedNames;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.visitCommandParams;
+import static org.apache.ignite.ssl.SslContextFactory.DFLT_SSL_PROTOCOL;
+
+/**
+ * Argument parser.
+ * Also would parse high-level command and delegate parsing for its argument 
to the command.
+ */
+public class ArgumentParser {
+/** */
+private final IgniteLogger log;
+
+/** */
+private final IgniteCommandRegistry registry;
+
+/** */
+static final String CMD_HOST = "--host";
+
+/** */
+static final Str

[GitHub] [ignite] nizhikov commented on a diff in pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

nizhikov commented on code in PR #10675:
URL: https://github.com/apache/ignite/pull/10675#discussion_r1227815722


##
modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/ArgumentParser.java:
##
@@ -0,0 +1,514 @@
+/*
+ * 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.commandline;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import org.apache.ignite.IgniteException;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.IgniteSystemProperties;
+import org.apache.ignite.internal.client.GridClientConfiguration;
+import org.apache.ignite.internal.commandline.argument.parser.CLIArgument;
+import 
org.apache.ignite.internal.commandline.argument.parser.CLIArgumentParser;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.IgniteCommandRegistry;
+import org.apache.ignite.internal.management.api.Argument;
+import org.apache.ignite.internal.management.api.ArgumentGroup;
+import org.apache.ignite.internal.management.api.BeforeNodeStartCommand;
+import org.apache.ignite.internal.management.api.CliSubcommandsWithPrefix;
+import org.apache.ignite.internal.management.api.Command;
+import org.apache.ignite.internal.management.api.CommandsRegistry;
+import org.apache.ignite.internal.management.api.ComputeCommand;
+import org.apache.ignite.internal.management.api.HelpCommand;
+import org.apache.ignite.internal.management.api.LocalCommand;
+import org.apache.ignite.internal.management.api.Positional;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.apache.ignite.lang.IgniteExperimental;
+import org.apache.ignite.ssl.SslContextFactory;
+
+import static 
org.apache.ignite.IgniteSystemProperties.IGNITE_ENABLE_EXPERIMENTAL_COMMAND;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_INTERVAL;
+import static 
org.apache.ignite.internal.client.GridClientConfiguration.DFLT_PING_TIMEOUT;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_HOST;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_PORT;
+import static 
org.apache.ignite.internal.commandline.CommandHandler.UTILITY_NAME;
+import static 
org.apache.ignite.internal.commandline.argument.parser.CLIArgument.optionalArg;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.CMD_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.NAME_PREFIX;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.PARAM_WORDS_DELIM;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.asOptional;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.fromFormattedCommandName;
+import static org.apache.ignite.internal.management.api.CommandUtils.isBoolean;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.parameterExample;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedCommandName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedFieldName;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.toFormattedNames;
+import static 
org.apache.ignite.internal.management.api.CommandUtils.visitCommandParams;
+import static org.apache.ignite.ssl.SslContextFactory.DFLT_SSL_PROTOCOL;
+
+/**
+ * Argument parser.
+ * Also would parse high-level command and delegate parsing for its argument 
to the command.
+ */
+public class ArgumentParser {
+/** */
+private final IgniteLogger log;
+
+/** */
+private final IgniteCommandRegistry registry;
+
+/** */
+static final String CMD_HOST = "--host";
+
+/** */
+static final Str

[GitHub] [ignite] nizhikov commented on a diff in pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

nizhikov commented on code in PR #10675:
URL: https://github.com/apache/ignite/pull/10675#discussion_r1227820159


##
modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/CommandInvoker.java:
##
@@ -0,0 +1,316 @@
+/*
+ * 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.commandline;
+
+import java.io.IOException;
+import java.net.InetAddress;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.ignite.IgniteLogger;
+import org.apache.ignite.internal.client.GridClient;
+import org.apache.ignite.internal.client.GridClientBeforeNodeStart;
+import org.apache.ignite.internal.client.GridClientCompute;
+import org.apache.ignite.internal.client.GridClientConfiguration;
+import org.apache.ignite.internal.client.GridClientDisconnectedException;
+import org.apache.ignite.internal.client.GridClientException;
+import org.apache.ignite.internal.client.GridClientFactory;
+import org.apache.ignite.internal.client.GridClientNode;
+import org.apache.ignite.internal.dto.IgniteDataTransferObject;
+import org.apache.ignite.internal.management.api.BeforeNodeStartCommand;
+import org.apache.ignite.internal.management.api.Command;
+import org.apache.ignite.internal.management.api.ComputeCommand;
+import org.apache.ignite.internal.management.api.LocalCommand;
+import org.apache.ignite.internal.management.api.PreparableCommand;
+import org.apache.ignite.internal.util.IgniteUtils;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.visor.VisorTaskArgument;
+import org.apache.ignite.lang.IgniteBiTuple;
+
+import static java.util.Collections.singleton;
+import static java.util.stream.Collectors.toMap;
+import static org.apache.ignite.internal.commandline.CommandHandler.DFLT_HOST;
+
+/**
+ * Adapter of new management API command for legacy {@code control.sh} 
execution flow.
+ */
+public class CommandInvoker {
+/** Command to execute. */
+private final Command cmd;
+
+/** Parsed argument. */
+private final A arg;
+
+/** Client configuration. */
+private GridClientConfiguration clientCfg;
+
+/** @param cmd Command to execute. */
+public CommandInvoker(Command cmd, A arg, GridClientConfiguration 
clientCfg) {
+this.cmd = cmd;
+this.arg = arg;
+this.clientCfg = clientCfg;
+}
+
+/**
+ * Actual command execution with verbose mode if needed.
+ * Implement it if your command supports verbose mode.
+ *
+ * @param log Logger to use.
+ * @param verbose Use verbose mode or not
+ * @return Result of operation (mostly usable for tests).
+ * @throws Exception If error occur.
+ */
+public  R invoke(IgniteLogger log, boolean verbose) throws Exception {
+try (GridClient client = startClient(clientCfg)) {
+String deprecationMsg = cmd.deprecationMessage(arg);
+
+if (deprecationMsg != null)
+log.warning(deprecationMsg);
+
+R res;
+
+if (cmd instanceof LocalCommand)
+res = ((LocalCommand)cmd).execute(client, arg, 
log::info);
+else if (cmd instanceof ComputeCommand) {
+GridClientCompute compute = client.compute();
+
+Map nodes = compute.nodes().stream()
+.collect(toMap(GridClientNode::nodeId, n -> n));
+
+ComputeCommand cmd = (ComputeCommand)this.cmd;
+
+Collection cmdNodes = cmd.nodes(nodes, arg);
+
+if (cmdNodes == null)
+cmdNodes = singleton(defaultNode(client, 
clientCfg).nodeId());
+
+for (UUID id : cmdNodes) {
+if (!nodes.containsKey(id))
+throw new IllegalArgumentException("Node with id=" + 
id + " not found.");
+}
+
+Collection connectable = F.viewReadOnly(
+cmdNodes,
+nodes::get,
+id -> nodes.get(id).connectable()
+);
+
+if (!F.isEmpty(connectable))
+  

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

sonarcloud[bot] commented on PR #10675:
URL: https://github.com/apache/ignite/pull/10675#issuecomment-1588919357

   SonarCloud Quality Gate failed.    [![Quality Gate 
failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png
 'Quality Gate 
failed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10675)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=BUG)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=BUG)
 [0 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10675&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10675&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10675&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=CODE_SMELL)
 [402 Code 
Smells](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10675&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10675&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10675&metric=new_coverage&view=list)
  
   
[![1.3%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'1.3%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10675&metric=new_duplicated_lines_density&view=list)
 [1.3% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10675&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] yurinaryshkin commented on a diff in pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

yurinaryshkin commented on code in PR #10767:
URL: https://github.com/apache/ignite/pull/10767#discussion_r1227840681


##
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryLimitTest.java:
##
@@ -0,0 +1,274 @@
+/*
+ * 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.cache.query;
+
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.IntUnaryOperator;
+import javax.cache.Cache;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.cache.query.annotations.QuerySqlField;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.processors.cache.query.QueryCursorEx;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheMode.REPLICATED;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.between;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.eq;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.gt;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.gte;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.in;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.lt;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.lte;
+
+/** */
+public class IndexQueryLimitTest extends GridCommonAbstractTest {
+/** */
+private static final String CACHE = "TEST_CACHE";
+
+/** */
+private static final String IDX = "PERSON_ID_IDX";
+
+/** */
+private static final int CNT = 10_000;
+
+/** */
+private Ignite crd;
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+crd = startGrids(4);
+}
+
+/** {@inheritDoc} */
+@Override protected void afterTest() {
+stopAllGrids();
+}
+
+/** {@inheritDoc} */
+@Override protected IgniteConfiguration getConfiguration(String 
igniteInstanceName) throws Exception {
+IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+CacheConfiguration ccfg = new CacheConfiguration()
+.setName(CACHE)
+.setIndexedTypes(Long.class, Person.class)
+.setAtomicityMode(TRANSACTIONAL)
+.setCacheMode(REPLICATED)
+.setQueryParallelism(1)
+.setBackups(0);
+
+cfg.setCacheConfiguration(ccfg);
+
+return cfg;
+}
+
+/** */
+@Test
+public void testRangeQueriesWithoutDuplicates() throws Exception {
+checkRangeQueries(1);
+}
+
+/** */
+@Test
+public void testRangeQueriesWithDuplicates() throws Exception {
+checkRangeQueries(10);
+}
+
+/** */
+@Test
+public void testSetLimit() throws Exception {
+GridTestUtils.assertThrows(log, () -> new IndexQuery<>(Person.class, 
IDX).setLimit(0),
+IllegalArgumentException.class, "Limit must be positive.");
+
+int limit = 1 + new Random().nextInt(1000);
+
+GridTestUtils.assertThrows(log, () -> new IndexQuery<>(Person.class, 
IDX).setLimit(0 - limit),
+IllegalArgumentException.class, "Limit must be positive.");
+
+IndexQuery qry = new IndexQuery<>(Person.class, IDX);
+
+qry.setLimit(limit);
+
+assertEquals(limit, qry.getLimit());
+}
+
+/** */
+private void checkRangeQueries(int duplicates) throws Exception {
+
+// Query empty cache.
+IndexQuery qry = new IndexQuery<>(Person.class, IDX);
+
+assertTrue(crd.cache(CACHE).query(qry).getAll().isEmpty());
+
+// Add data
+insertData(duplicates);
+
+// All
+ch

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

sonarcloud[bot] commented on PR #10767:
URL: https://github.com/apache/ignite/pull/10767#issuecomment-1588956393

   Kudos, SonarCloud Quality Gate passed!    [![Quality Gate 
passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png
 'Quality Gate 
passed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10767)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
 [0 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
 [1 Code 
Smell](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_coverage&view=list)
  
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_duplicated_lines_density&view=list)
 [0.0% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2178: IGNITE-19535 Removed a requirement to have a special constructor for Ignite exceptions

[via GitHub]

ibessonov commented on code in PR #2178:
URL: https://github.com/apache/ignite-3/pull/2178#discussion_r1227754082


##
modules/api/src/main/java/org/apache/ignite/lang/IgniteExceptionUtils.java:
##
@@ -0,0 +1,553 @@
+/*
+ * 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.lang;
+
+import static java.lang.invoke.MethodHandles.lookup;
+import static java.lang.invoke.MethodHandles.privateLookupIn;
+import static java.lang.invoke.MethodHandles.publicLookup;
+import static java.lang.invoke.MethodType.methodType;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodType;
+import java.lang.invoke.WrongMethodTypeException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.UUID;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import org.apache.ignite.internal.util.ExceptionUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility class to wrap Ignite exceptions.
+ * TODO: https://issues.apache.org/jira/browse/IGNITE-19566 this class should 
be merged with ExceptionUtils.
+ */
+public class IgniteExceptionUtils {
+/** Private constructor to prohibit creating an instance of utility class. 
*/
+private IgniteExceptionUtils() {

Review Comment:
   I ask this everyone who writes such code.
   Have you ever encounter anyone who tried instantiating utility class? I see 
these constructors as utterly pointless.



##
modules/api/src/main/java/org/apache/ignite/lang/IgniteExceptionUtils.java:
##
@@ -0,0 +1,553 @@
+/*
+ * 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.lang;
+
+import static java.lang.invoke.MethodHandles.lookup;
+import static java.lang.invoke.MethodHandles.privateLookupIn;
+import static java.lang.invoke.MethodHandles.publicLookup;
+import static java.lang.invoke.MethodType.methodType;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodType;
+import java.lang.invoke.WrongMethodTypeException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.UUID;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import org.apache.ignite.internal.util.ExceptionUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility class to wrap Ignite exceptions.
+ * TODO: https://issues.apache.org/jira/browse/IGNITE-19566 this class should 
be merged with ExceptionUtils.
+ */
+public class IgniteExceptionUtils {
+/** Private constructor to prohibit creating an instance of utility class. 
*/
+private IgniteExceptionUtils() {
+// No-op.
+}
+
+/**
+ * Creates and returns a copy of an exception that is a cause of the given 
{@code CompletionException}.
+ * If the original exception does not contain a cause, then the original 
exception will be returned.
+ * In order to preserve a stack trace, the original completion exception 
will be set as the cause of the newly created exception.
+ * 
+ * For example, this method might be useful when you need to implement 
sync API over async one.
+ * 
+ * 
+ * public CompletableFuture asyncMethod {...}
+ *
+ * public Result syncMethod(

[GitHub] [ignite-3] PakhomovAlexander merged pull request #2127: IGNITE-19552 Build JDBC module jar

[via GitHub]

PakhomovAlexander merged PR #2127:
URL: https://github.com/apache/ignite-3/pull/2127


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sk0x50 commented on a diff in pull request #2178: IGNITE-19535 Removed a requirement to have a special constructor for Ignite exceptions

[via GitHub]

sk0x50 commented on code in PR #2178:
URL: https://github.com/apache/ignite-3/pull/2178#discussion_r1227877049


##
modules/api/src/main/java/org/apache/ignite/lang/IgniteExceptionUtils.java:
##
@@ -0,0 +1,553 @@
+/*
+ * 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.lang;
+
+import static java.lang.invoke.MethodHandles.lookup;
+import static java.lang.invoke.MethodHandles.privateLookupIn;
+import static java.lang.invoke.MethodHandles.publicLookup;
+import static java.lang.invoke.MethodType.methodType;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodType;
+import java.lang.invoke.WrongMethodTypeException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.UUID;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import org.apache.ignite.internal.util.ExceptionUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility class to wrap Ignite exceptions.
+ * TODO: https://issues.apache.org/jira/browse/IGNITE-19566 this class should 
be merged with ExceptionUtils.
+ */
+public class IgniteExceptionUtils {
+/** Private constructor to prohibit creating an instance of utility class. 
*/
+private IgniteExceptionUtils() {

Review Comment:
   Well, it is an interesting question :)
   It looks absolutely reasonable to prohibit an action that does not make any 
sense. On the other hand, it does not break anything and so, this private 
constructor can be omitted.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sk0x50 commented on a diff in pull request #2178: IGNITE-19535 Removed a requirement to have a special constructor for Ignite exceptions

[via GitHub]

sk0x50 commented on code in PR #2178:
URL: https://github.com/apache/ignite-3/pull/2178#discussion_r1227878072


##
modules/api/src/main/java/org/apache/ignite/lang/IgniteExceptionUtils.java:
##
@@ -0,0 +1,553 @@
+/*
+ * 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.lang;
+
+import static java.lang.invoke.MethodHandles.lookup;
+import static java.lang.invoke.MethodHandles.privateLookupIn;
+import static java.lang.invoke.MethodHandles.publicLookup;
+import static java.lang.invoke.MethodType.methodType;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodType;
+import java.lang.invoke.WrongMethodTypeException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.UUID;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.ExecutionException;
+import org.apache.ignite.internal.util.ExceptionUtils;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Utility class to wrap Ignite exceptions.
+ * TODO: https://issues.apache.org/jira/browse/IGNITE-19566 this class should 
be merged with ExceptionUtils.
+ */
+public class IgniteExceptionUtils {
+/** Private constructor to prohibit creating an instance of utility class. 
*/
+private IgniteExceptionUtils() {
+// No-op.
+}
+
+/**
+ * Creates and returns a copy of an exception that is a cause of the given 
{@code CompletionException}.
+ * If the original exception does not contain a cause, then the original 
exception will be returned.
+ * In order to preserve a stack trace, the original completion exception 
will be set as the cause of the newly created exception.
+ * 
+ * For example, this method might be useful when you need to implement 
sync API over async one.
+ * 
+ * 
+ * public CompletableFuture asyncMethod {...}
+ *
+ * public Result syncMethod() {
+ * try {
+ * return asyncMethod.join();
+ * } catch (CompletionException e) {
+ * throw sneakyThrow(createCopyExceptionWithCause(e));
+ * }
+ * }
+ *
+ * 
+ *
+ * @param exception Original completion exception.
+ * @return Copy of an exception that is a cause of the given {@code 
CompletionException}.
+ */
+public static Throwable createCopyExceptionWithCause(CompletionException 
exception) {
+return createCopyExceptionWithCauseInternal(exception);
+}
+
+/**
+ * Creates and returns a copy of an exception that is a cause of the given 
{@code ExecutionException}.
+ * If the original exception does not contain a cause, then the original 
exception will be returned.
+ * In order to preserve a stack trace, the original completion exception 
will be set as the cause of the newly created exception.
+ * 
+ * For example, this method might be useful when you need to implement 
sync API over async one.
+ * 
+ * 
+ * public CompletableFuture asyncMethod {...}
+ *
+ * public Result syncMethod() {
+ * try {
+ * return asyncMethod.get();
+ * } catch (ExecutionException e) {
+ * throw sneakyThrow(createCopyExceptionWithCause(e));
+ * }
+ * }
+ *
+ * 
+ *
+ * @param exception Original execution exception.
+ * @return Copy of an exception that is a cause of the given {@code 
ExecutionException}.
+ */
+public static Throwable createCopyExceptionWithCause(ExecutionException 
exception) {
+return createCopyExceptionWithCauseInternal(exception);
+}
+
+/**
+ * Creates and returns a new exception of the given {@code clazz} and with 
the specified parameters.
+ *
+ * @param clazz Class of the required exception to be instantiated.
+ * @param traceId Trace identifier.
+ * @param code Error code.
+ * @param message Error message.
+ * @param cause Cause.
+ * @return New exception of the given {@code clazz} and with the specified 
parameters.
+ */
+public static  T create

[GitHub] [ignite-3] sk0x50 commented on a diff in pull request #2178: IGNITE-19535 Removed a requirement to have a special constructor for Ignite exceptions

[via GitHub]

sk0x50 commented on code in PR #2178:
URL: https://github.com/apache/ignite-3/pull/2178#discussion_r1227879844


##
modules/api/src/main/java/org/apache/ignite/lang/IgniteExceptionUtils.java:
##
@@ -0,0 +1,553 @@
+/*
+ * 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.lang;

Review Comment:
   This class should be moved into the `internal` package of course. This issue 
will be addressed by https://issues.apache.org/jira/browse/IGNITE-19566



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] nizhikov merged pull request #10675: IGNITE-15629 Management API introduced

[via GitHub]

nizhikov merged PR #10675:
URL: https://github.com/apache/ignite/pull/10675


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] alievmirza opened a new pull request, #2185: IGNITE-19714 Fix AssertionError in DistributionZoneManagerWatchListenerTest

[via GitHub]

alievmirza opened a new pull request, #2185:
URL: https://github.com/apache/ignite-3/pull/2185

   https://issues.apache.org/jira/browse/IGNITE-19714


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] timoninmaxim commented on a diff in pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

timoninmaxim commented on code in PR #10767:
URL: https://github.com/apache/ignite/pull/10767#discussion_r1227906760


##
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryLimitTest.java:
##
@@ -113,18 +110,14 @@ public void testSetLimit() throws Exception {
 
 /** */
 private void checkRangeQueries(int duplicates) throws Exception {
-// Query empty cache.
 IndexQuery qry = new IndexQuery<>(Person.class, IDX);

Review Comment:
   Unused variable



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] timoninmaxim commented on a diff in pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

timoninmaxim commented on code in PR #10767:
URL: https://github.com/apache/ignite/pull/10767#discussion_r1227909206


##
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryLimitTest.java:
##
@@ -0,0 +1,238 @@
+/*
+ * 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.cache.query;
+
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.cache.Cache;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.cache.query.annotations.QuerySqlField;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.processors.cache.query.QueryCursorEx;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheMode.REPLICATED;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.lt;
+
+/** */
+public class IndexQueryLimitTest extends GridCommonAbstractTest {
+/** */
+private static final String CACHE = "TEST_CACHE";
+
+/** */
+private static final String IDX = "PERSON_ID_IDX";
+
+/** */
+private static final int CNT = 10_000;
+
+/** */
+private Ignite crd;
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+crd = startGrids(4);
+}
+
+/** {@inheritDoc} */
+@Override protected void afterTest() {
+stopAllGrids();
+}
+
+/** {@inheritDoc} */
+@Override protected IgniteConfiguration getConfiguration(String 
igniteInstanceName) throws Exception {
+IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+CacheConfiguration ccfg = new CacheConfiguration()
+.setName(CACHE)
+.setIndexedTypes(Long.class, Person.class)
+.setAtomicityMode(TRANSACTIONAL)
+.setCacheMode(REPLICATED);
+
+cfg.setCacheConfiguration(ccfg);
+
+return cfg;
+}
+
+/** */
+@Test
+public void testRangeQueriesWithoutDuplicates() throws Exception {
+checkRangeQueries(1);
+}
+
+/** */
+@Test
+public void testRangeQueriesWithDuplicates() throws Exception {
+checkRangeQueries(10);
+}
+
+/** */
+@Test
+public void testSetLimit() throws Exception {
+GridTestUtils.assertThrows(log, () -> new IndexQuery<>(Person.class, 
IDX).setLimit(0),
+IllegalArgumentException.class, "Limit must be positive.");
+
+int limit = 1 + new Random().nextInt(1000);
+
+GridTestUtils.assertThrows(log, () -> new IndexQuery<>(Person.class, 
IDX).setLimit(0 - limit),
+IllegalArgumentException.class, "Limit must be positive.");
+
+IndexQuery qry = new IndexQuery<>(Person.class, IDX);
+
+qry.setLimit(limit);
+
+assertEquals(limit, qry.getLimit());
+}
+
+/** */
+private void checkRangeQueries(int duplicates) throws Exception {
+IndexQuery qry = new IndexQuery<>(Person.class, IDX);
+
+// Add data
+insertData(duplicates);
+
+// All
+checkLimit(null, 0, CNT, duplicates);
+
+String fld = "id";
+
+int pivot = new Random().nextInt(CNT);
+
+// Lt.
+checkLimit(lt(fld, pivot), 0, pivot, duplicates);
+

Review Comment:
   Please remove empty line



##
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryLimitTest.java:
##
@@ -0,0 +1,238 @@
+/*
+ * 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 complia

[GitHub] [ignite] timoninmaxim commented on a diff in pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

timoninmaxim commented on code in PR #10767:
URL: https://github.com/apache/ignite/pull/10767#discussion_r1227912089


##
modules/indexing/src/test/java/org/apache/ignite/cache/query/ThinClientIndexQueryTest.java:
##
@@ -369,6 +393,47 @@ private void assertClientQuery(ClientCache cache, int left, int
 }
 }
 
+/** */
+@Test
+public void testIndexQueryLimitOnOlderProtocolVersion() throws Exception {
+// Exclude INDEX_QUERY_LIMIT from protocol

Review Comment:
   All comments must ends with a dot. 



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] timoninmaxim commented on a diff in pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

timoninmaxim commented on code in PR #10767:
URL: https://github.com/apache/ignite/pull/10767#discussion_r1227914726


##
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryLimitTest.java:
##
@@ -0,0 +1,238 @@
+/*
+ * 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.cache.query;
+
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.cache.Cache;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.cache.query.annotations.QuerySqlField;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.processors.cache.query.QueryCursorEx;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheMode.REPLICATED;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.lt;
+
+/** */
+public class IndexQueryLimitTest extends GridCommonAbstractTest {
+/** */
+private static final String CACHE = "TEST_CACHE";
+
+/** */
+private static final String IDX = "PERSON_ID_IDX";
+
+/** */
+private static final int CNT = 10_000;
+
+/** */
+private Ignite crd;
+
+/** {@inheritDoc} */
+@Override protected void beforeTest() throws Exception {
+crd = startGrids(4);
+}
+
+/** {@inheritDoc} */
+@Override protected void afterTest() {
+stopAllGrids();
+}
+
+/** {@inheritDoc} */
+@Override protected IgniteConfiguration getConfiguration(String 
igniteInstanceName) throws Exception {
+IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+CacheConfiguration ccfg = new CacheConfiguration()
+.setName(CACHE)
+.setIndexedTypes(Long.class, Person.class)
+.setAtomicityMode(TRANSACTIONAL)
+.setCacheMode(REPLICATED);
+
+cfg.setCacheConfiguration(ccfg);
+
+return cfg;
+}
+
+/** */
+@Test
+public void testRangeQueriesWithoutDuplicates() throws Exception {
+checkRangeQueries(1);
+}
+
+/** */
+@Test
+public void testRangeQueriesWithDuplicates() throws Exception {
+checkRangeQueries(10);
+}
+
+/** */
+@Test
+public void testSetLimit() throws Exception {

Review Comment:
   Exception doesn't throw



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10767: IGNITE-16619 IndexQuery should support limit

[via GitHub]

sonarcloud[bot] commented on PR #10767:
URL: https://github.com/apache/ignite/pull/10767#issuecomment-1589086329

   Kudos, SonarCloud Quality Gate passed!    [![Quality Gate 
passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png
 'Quality Gate 
passed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10767)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
 [0 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10767&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
 [1 Code 
Smell](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10767&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_coverage&view=list)
  
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_duplicated_lines_density&view=list)
 [0.0% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10767&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] ivandasch commented on a diff in pull request #10769: IGNITE-19588 SQL Calcite: Add long-running queries warnings

[via GitHub]

ivandasch commented on code in PR #10769:
URL: https://github.com/apache/ignite/pull/10769#discussion_r1227953672


##
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessor.java:
##
@@ -534,7 +519,7 @@ private  T processQuery(
 params,
 qryCtx,
 exchangeSvc,
-(q) -> qryReg.unregister(q.id()),
+(q, f) -> qryReg.unregister(q.id(), f),

Review Comment:
   It seems that it is better to rename `f` to `ex`



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10638: IGNITE-19164 Improve message about requested partitions during snapshot restore

[via GitHub]

sonarcloud[bot] commented on PR #10638:
URL: https://github.com/apache/ignite/pull/10638#issuecomment-1589144139

   Kudos, SonarCloud Quality Gate passed!    [![Quality Gate 
passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png
 'Quality Gate 
passed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10638)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=BUG)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=BUG)
 [0 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10638&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10638&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10638&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=CODE_SMELL)
 [1 Code 
Smell](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10638&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10638&metric=new_coverage&view=list)
  
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10638&metric=new_duplicated_lines_density&view=list)
 [0.0% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10638&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] ptupitsyn merged pull request #2182: IGNITE-19545 .NET: Add Basic Data Streamer

[via GitHub]

ptupitsyn merged PR #2182:
URL: https://github.com/apache/ignite-3/pull/2182


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2171: IGNITE-19600 Removed topologyVersionedDataNodes

[via GitHub]

kgusakov commented on code in PR #2171:
URL: https://github.com/apache/ignite-3/pull/2171#discussion_r1228023852


##
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerLogicalTopologyEventsTest.java:
##
@@ -45,6 +48,9 @@ void testMetaStorageKeysInitializedOnStartWhenTopVerEmpty() 
throws Exception {
 
 distributionZoneManager.start();
 
+verify(keyValueStorage, timeout(1000).times(1))

Review Comment:
   It was here before the topologyVersioneDataNodes logic 
https://github.com/apache/ignite-3/pull/1729/files#diff-eee737dc0330b6a383f01df9ee1d1ae3135655204bdb1a261ff874674d4a6a78L190
 - I just thought, that the author of this test earlier wanted to check that 
the call of keyValueStorage occuried during init. 



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] kgusakov commented on a diff in pull request #2171: IGNITE-19600 Removed topologyVersionedDataNodes

[via GitHub]

kgusakov commented on code in PR #2171:
URL: https://github.com/apache/ignite-3/pull/2171#discussion_r1228028031


##
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerLogicalTopologyEventsTest.java:
##
@@ -45,6 +48,9 @@ void testMetaStorageKeysInitializedOnStartWhenTopVerEmpty() 
throws Exception {
 
 distributionZoneManager.start();
 
+verify(keyValueStorage, timeout(1000).times(1))

Review Comment:
   Sorry, wrong test, I think I put it here by mistake, will remove it



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] vldpyatkov merged pull request #2157: IGNITE-18692 ItRebalanceTest fix

[via GitHub]

vldpyatkov merged PR #2157:
URL: https://github.com/apache/ignite-3/pull/2157


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10638: IGNITE-19164 Improve message about requested partitions during snapshot restore

[via GitHub]

sonarcloud[bot] commented on PR #10638:
URL: https://github.com/apache/ignite/pull/10638#issuecomment-1589205779

   Kudos, SonarCloud Quality Gate passed!    [![Quality Gate 
passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png
 'Quality Gate 
passed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10638)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=BUG)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=BUG)
 [0 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10638&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10638&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10638&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=CODE_SMELL)
 [1 Code 
Smell](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10638&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10638&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10638&metric=new_coverage&view=list)
  
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10638&metric=new_duplicated_lines_density&view=list)
 [0.0% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10638&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] zstan commented on pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping

[via GitHub]

zstan commented on PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#issuecomment-1589237743

   @korlov42 i revert tx fix and change only mapping part, can u take a look ?


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] PakhomovAlexander merged pull request #2152: IGNITE-19522 Modify deploy unit command

[via GitHub]

PakhomovAlexander merged PR #2152:
URL: https://github.com/apache/ignite-3/pull/2152


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping

[via GitHub]

korlov42 commented on code in PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#discussion_r1228113151


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java:
##
@@ -85,33 +231,19 @@ public MetadataDef getDef() {
  * @param mq  Metadata query instance. Used to request appropriate 
metadata from node children.
  * @return Nodes mapping, representing a list of nodes capable to execute 
a query over particular partitions.
  */
-public FragmentMapping fragmentMapping(RelNode rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
-throw new AssertionError();
-}
-
-/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
- */
-public FragmentMapping fragmentMapping(RelSubset rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
-throw new AssertionError();
-}
-
-/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
- */
-public FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
+private static FragmentMapping fragmentMapping(SingleRel rel, 
RelMetadataQuery mq, MappingQueryContext ctx) {
 return fragmentMappingForMetadataQuery(rel.getInput(), mq, ctx);
 }
 
 /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
+ * See {@link IgniteFragmentMapping#fragmentMapping(SingleRel, 
RelMetadataQuery, MappingQueryContext)}.
  *
  * {@link ColocationMappingException} may be thrown on two children 
nodes locations merge. This means that the fragment
  * (which part the parent node is) cannot be executed on any node and 
additional exchange is needed. This case we throw {@link
  * NodeMappingException} with an edge, where we need the additional 
exchange. After the exchange is put into the fragment and the
  * fragment is split into two ones, fragment meta information will be 
recalculated for all fragments.
  */
-public FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
+private static FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery 
mq, MappingQueryContext ctx) {

Review Comment:
   why do we need all those static fields? It's better to inline them



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping

[via GitHub]

korlov42 commented on code in PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#discussion_r1228113886


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java:
##
@@ -69,13 +75,153 @@ public class IgniteMdFragmentMapping implements 
MetadataHandler

[GitHub] [ignite] asfgit closed pull request #10769: IGNITE-19588 SQL Calcite: Add long-running queries warnings

[via GitHub]

asfgit closed pull request #10769: IGNITE-19588 SQL Calcite: Add long-running 
queries warnings
URL: https://github.com/apache/ignite/pull/10769


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10769: IGNITE-19588 SQL Calcite: Add long-running queries warnings

[via GitHub]

sonarcloud[bot] commented on PR #10769:
URL: https://github.com/apache/ignite/pull/10769#issuecomment-1589314034

   SonarCloud Quality Gate failed.    [![Quality Gate 
failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png
 'Quality Gate 
failed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10769)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=BUG)
 
[![E](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E-16px.png
 
'E')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=BUG)
 [5 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10769&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10769&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10769&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=CODE_SMELL)
 [55 Code 
Smells](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10769&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10769&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10769&metric=new_coverage&view=list)
  
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10769&metric=new_duplicated_lines_density&view=list)
 [0.0% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10769&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] rpuch commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

rpuch commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228074486


##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##
@@ -1031,10 +1038,10 @@ public void handle(VersionedUpdate update) {
 : new 
CatalogTableDescriptor(
 table.id(),
 table.name(),
+table.zoneId(),
 
table.columns().stream()
-
.map(source -> source.name().equals(target.name())
-? 
target
-: 
source).collect(Collectors.toList()),
+
.map(source -> source.name().equals(target.name()) ? target : source)
+
.collect(Collectors.toList()),

Review Comment:
   How about a static import?



##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogDescriptorUtils.java:
##
@@ -188,4 +211,21 @@ private static CatalogIndexColumnDescriptor 
toIndexColumnDescriptor(IndexColumnV
 
 return new CatalogIndexColumnDescriptor(config.name(), collation);
 }
+
+// TODO: IGNITE-19719 Fix it
+private static CatalogDataStorageDescriptor 
toDataStorageDescriptor(DataStorageView config) {
+String dataRegion;
+
+try {
+Method dataRegionMethod = 
config.getClass().getMethod("dataRegion");

Review Comment:
   Is this caused by concrete configuration classes (rocksdb/aipersist/aimem) 
inavailability? How will it work when we move from the configuration 
completely? How will such polymorphic descriptors be built?



##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java:
##
@@ -75,6 +78,35 @@ public CatalogZoneDescriptor(
 this.filter = filter;
 }
 
+/**
+ * Constructs a distribution zone descriptor.
+ *
+ * @param id Id of the distribution zone.
+ * @param name Name of the zone.
+ * @param partitions Amount of partitions in distributions zone.
+ * @param replicas Amount of partition replicas.

Review Comment:
   ```suggestion
* @param partitions Number of partitions in distributions zone.
* @param replicas Number of partition replicas.
   ```



##
modules/catalog/build.gradle:
##
@@ -29,6 +29,7 @@ dependencies {
 implementation project(':ignite-metastorage-api')
 implementation project(':ignite-vault')
 implementation project(':ignite-schema')
+implementation project(':ignite-distribution-zones')

Review Comment:
   Why does the catalog module need to depend on the distribution zones module? 
Is it a temporary solution (until we stop using configurations to store 
schemas)?



##
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##
@@ -154,21 +140,11 @@ protected final void initialize(
 }
 
 @AfterEach
-void tearDown() throws Exception {
-IgniteUtils.closeAll(
-tableStorage == null ? null : tableStorage::stop,
-storageEngine == null ? null : storageEngine::stop
-);
-}
-
-protected MvTableStorage createMvTableStorage(TablesConfiguration 
tablesConfig,
-DistributionZoneConfiguration distributionZoneConfiguration) {
-return storageEngine.createMvTable(getTableConfig(tablesConfig), 
tablesConfig, distributionZoneConfiguration);
+protected void tearDown() throws Exception {
+IgniteUtils.closeAllManually(tableStorage);

Review Comment:
   How about calling `close()` directly here? `closeAll()` makes sense when 
there are at least 2 things to close



##
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##
@@ -1408,55 +1419,38 @@ public void testDefaultZone() {
 
 // Validate default zone wasn't changed.
 assertSame(defaultZone, service.zone(CatalogService.DEFAULT_ZONE_NAME, 
clock.nowLong()));
-
-// Try to rename to a zone with default name.

Review Comment:
   Was this scenario moved elsewhere? Or we don't need it amymore?



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apa

[GitHub] [ignite] petrov-mg opened a new pull request, #10770: IGNITE-19715 Thin client socket#open timeout changed to ClientConfiguration#timeout property.

[via GitHub]

petrov-mg opened a new pull request, #10770:
URL: https://github.com/apache/ignite/pull/10770

   
   
   Thank you for submitting the pull request to the Apache Ignite.
   
   In order to streamline the review of the contribution 
   we ask you to ensure the following steps have been taken:
   
   ### The Contribution Checklist
   - [ ] There is a single JIRA ticket related to the pull request. 
   - [ ] The web-link to the pull request is attached to the JIRA ticket.
   - [ ] The JIRA ticket has the _Patch Available_ state.
   - [ ] The pull request body describes changes that have been made. 
   The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
   - [ ] The pull request title is treated as the final commit message. 
   The following pattern must be used: `IGNITE- Change summary` where 
`` - number of JIRA issue.
   - [ ] A reviewer has been mentioned through the JIRA comments 
   (see [the Maintainers 
list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
 
   - [ ] The pull request has been checked by the Teamcity Bot and 
   the `green visa` attached to the JIRA ticket (see [TC.Bot: Check 
PR](https://mtcga.gridgain.com/prs.html))
   
   ### Notes
   - [How to 
Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
   - [Coding abbreviation 
rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
   - [Coding 
Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
   - [Apache Ignite Teamcity 
Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
   
   If you need any help, please email d...@ignite.apache.org or ask anу advice 
on http://asf.slack.com _#ignite_ channel.
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228209827


##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogServiceImpl.java:
##
@@ -1031,10 +1038,10 @@ public void handle(VersionedUpdate update) {
 : new 
CatalogTableDescriptor(
 table.id(),
 table.name(),
+table.zoneId(),
 
table.columns().stream()
-
.map(source -> source.name().equals(target.name())
-? 
target
-: 
source).collect(Collectors.toList()),
+
.map(source -> source.name().equals(target.name()) ? target : source)
+
.collect(Collectors.toList()),

Review Comment:
   fix it



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sk0x50 opened a new pull request, #2186: IGNITE-19536 Added RecoverableException marker interface indicates recoverable error

[via GitHub]

sk0x50 opened a new pull request, #2186:
URL: https://github.com/apache/ignite-3/pull/2186

   https://issues.apache.org/jira/browse/IGNITE-19536


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10770: IGNITE-19715 Thin client socket#open timeout changed to ClientConfiguration#timeout property.

[via GitHub]

sonarcloud[bot] commented on PR #10770:
URL: https://github.com/apache/ignite/pull/10770#issuecomment-1589412385

   Kudos, SonarCloud Quality Gate passed!    [![Quality Gate 
passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png
 'Quality Gate 
passed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10770)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=BUG)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=BUG)
 [0 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10770&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10770&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10770&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=CODE_SMELL)
 [0 Code 
Smells](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10770&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10770&metric=new_coverage&view=list)
  
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10770&metric=new_duplicated_lines_density&view=list)
 [0.0% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10770&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228213949


##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogDescriptorUtils.java:
##
@@ -188,4 +211,21 @@ private static CatalogIndexColumnDescriptor 
toIndexColumnDescriptor(IndexColumnV
 
 return new CatalogIndexColumnDescriptor(config.name(), collation);
 }
+
+// TODO: IGNITE-19719 Fix it
+private static CatalogDataStorageDescriptor 
toDataStorageDescriptor(DataStorageView config) {
+String dataRegion;
+
+try {
+Method dataRegionMethod = 
config.getClass().getMethod("dataRegion");

Review Comment:
   Yes, this "great" solution came from the unavailability of these engines, 
and at the moment I would not really like to add them to the dependency, since 
it will turn out to be a cyclic dependency.
   
   I created IGNITE-19719 to figure out how we can do well.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228215004


##
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java:
##
@@ -75,6 +78,35 @@ public CatalogZoneDescriptor(
 this.filter = filter;
 }
 
+/**
+ * Constructs a distribution zone descriptor.
+ *
+ * @param id Id of the distribution zone.
+ * @param name Name of the zone.
+ * @param partitions Amount of partitions in distributions zone.
+ * @param replicas Amount of partition replicas.

Review Comment:
   fix it



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228216385


##
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogServiceSelfTest.java:
##
@@ -1408,55 +1419,38 @@ public void testDefaultZone() {
 
 // Validate default zone wasn't changed.
 assertSame(defaultZone, service.zone(CatalogService.DEFAULT_ZONE_NAME, 
clock.nowLong()));
-
-// Try to rename to a zone with default name.

Review Comment:
   This code is similar to copy-paste, this scenario is checked above.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228217596


##
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##
@@ -154,21 +140,11 @@ protected final void initialize(
 }
 
 @AfterEach
-void tearDown() throws Exception {
-IgniteUtils.closeAll(
-tableStorage == null ? null : tableStorage::stop,
-storageEngine == null ? null : storageEngine::stop
-);
-}
-
-protected MvTableStorage createMvTableStorage(TablesConfiguration 
tablesConfig,
-DistributionZoneConfiguration distributionZoneConfiguration) {
-return storageEngine.createMvTable(getTableConfig(tablesConfig), 
tablesConfig, distributionZoneConfiguration);
+protected void tearDown() throws Exception {
+IgniteUtils.closeAllManually(tableStorage);

Review Comment:
   Fix it



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228220452


##
modules/catalog/build.gradle:
##
@@ -29,6 +29,7 @@ dependencies {
 implementation project(':ignite-metastorage-api')
 implementation project(':ignite-vault')
 implementation project(':ignite-schema')
+implementation project(':ignite-distribution-zones')

Review Comment:
   This is a temporary dependency so that we can convert the table/index/zone 
configuration in "one place".
   
   When we leave the configuration, this dependence should disappear in theory.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2146: IGNITE-19517 Cache JobClassLoaders

[via GitHub]

PakhomovAlexander commented on code in PR #2146:
URL: https://github.com/apache/ignite-3/pull/2146#discussion_r1227119099


##
modules/code-deployment/src/main/java/org/apache/ignite/internal/deployunit/DeploymentManagerImpl.java:
##
@@ -296,6 +319,41 @@ public CompletableFuture onDemandDeploy(String 
id, Version version) {
 }
 return messaging.downloadUnitContent(id, version, nodes)
 .thenCompose(unitContent -> deployToLocalNode(id, 
version, unitContent));
+})
+.thenCompose(result -> {
+if (result) {
+return completedFuture(null);
+} else {
+LOG.error("Failed to deploy on demand deployment unit 
{}:{}", id, version);
+return clusterStatusAsync(id, version)
+.thenCombine(nodeStatusAsync(id, version), 
(clusterStatus, nodeStatus) -> {
+if (clusterStatus == null) {
+return 
CompletableFuture.failedFuture(
+new 
DeploymentUnitNotFoundException(id, version));
+} else {
+return 
CompletableFuture.failedFuture(
+new 
DeploymentUnitUnavailableException(
+id,
+version,
+clusterStatus,
+nodeStatus
+)
+);
+}
+}).thenCompose(it -> it);
+}
+});
+}
+
+@Override
+public CompletableFuture detectLatestDeployedVersion(String id) {

Review Comment:
   the method contains only "Deployed" word but the filter function includes 
DEPLOYED or UPLOADING status. That is confusing.



##
modules/compute/src/main/java/org/apache/ignite/internal/compute/ClassLoaderExceptionsMapper.java:
##
@@ -0,0 +1,82 @@
+/*
+ * 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.compute;
+
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import org.apache.ignite.internal.compute.loader.JobContext;
+import 
org.apache.ignite.internal.deployunit.exception.DeploymentUnitNotFoundException;
+import 
org.apache.ignite.internal.deployunit.exception.DeploymentUnitUnavailableException;
+
+class ClassLoaderExceptionsMapper {
+// . Deployment unit  doesn’t 
exist.
+private static final String DEPLOYMENT_UNIT_DOES_NOT_EXIST_MSG = "%s. 
Deployment unit %s:%s doesn’t exist";
+
+
+// . Deployment unit  can’t be used:
+// [clusterStatus = , nodeStatus = 
].
+private static final String DEPLOYMENT_UNIT_NOT_AVAILABLE_MSG = "%s. 
Deployment unit %s:%s can’t be used: "
++ "[clusterStatus = %s, nodeStatus = %s]";
+
+static CompletableFuture mapClassLoaderExceptions(
+CompletableFuture future,
+String jobClassName
+) {
+return future.handle((v, e) -> {
+if (e instanceof Exception) {
+throw new 
CompletionException(mapException(unwrapCompletionException((Exception) e), 
jobClassName));
+} else {
+return v;

Review Comment:
   Are you sure you do not want to log the exception at least? What if it is an 
important error? 



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] PakhomovAlexander merged pull request #2146: IGNITE-19517 Cache JobClassLoaders

[via GitHub]

PakhomovAlexander merged PR #2146:
URL: https://github.com/apache/ignite-3/pull/2146


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] Flaugh24 commented on a diff in pull request #2146: IGNITE-19517 Cache JobClassLoaders

[via GitHub]

Flaugh24 commented on code in PR #2146:
URL: https://github.com/apache/ignite-3/pull/2146#discussion_r1228238924


##
modules/compute/src/main/java/org/apache/ignite/internal/compute/ClassLoaderExceptionsMapper.java:
##
@@ -0,0 +1,82 @@
+/*
+ * 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.compute;
+
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import org.apache.ignite.internal.compute.loader.JobContext;
+import 
org.apache.ignite.internal.deployunit.exception.DeploymentUnitNotFoundException;
+import 
org.apache.ignite.internal.deployunit.exception.DeploymentUnitUnavailableException;
+
+class ClassLoaderExceptionsMapper {
+// . Deployment unit  doesn’t 
exist.
+private static final String DEPLOYMENT_UNIT_DOES_NOT_EXIST_MSG = "%s. 
Deployment unit %s:%s doesn’t exist";
+
+
+// . Deployment unit  can’t be used:
+// [clusterStatus = , nodeStatus = 
].
+private static final String DEPLOYMENT_UNIT_NOT_AVAILABLE_MSG = "%s. 
Deployment unit %s:%s can’t be used: "
++ "[clusterStatus = %s, nodeStatus = %s]";
+
+static CompletableFuture mapClassLoaderExceptions(
+CompletableFuture future,
+String jobClassName
+) {
+return future.handle((v, e) -> {
+if (e instanceof Exception) {
+throw new 
CompletionException(mapException(unwrapCompletionException((Exception) e), 
jobClassName));
+} else {
+return v;

Review Comment:
   I don't think the mapper should be responsible for that. We have logging at 
other levels. 



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] rpuch commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

rpuch commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228241311


##
modules/catalog/build.gradle:
##
@@ -29,6 +29,7 @@ dependencies {
 implementation project(':ignite-metastorage-api')
 implementation project(':ignite-vault')
 implementation project(':ignite-schema')
+implementation project(':ignite-distribution-zones')

Review Comment:
   I think it's worth adding a TODO to this file then, to make sure we don't 
forget to remove the dependency



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

tkalkirill commented on code in PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184#discussion_r1228254928


##
modules/catalog/build.gradle:
##
@@ -29,6 +29,7 @@ dependencies {
 implementation project(':ignite-metastorage-api')
 implementation project(':ignite-vault')
 implementation project(':ignite-schema')
+implementation project(':ignite-distribution-zones')

Review Comment:
   fix it



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228259420


##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();

Review Comment:
   AFAIK it's not valid to use var here.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228270424


##
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/MetaStorageManager.java:
##
@@ -170,8 +170,10 @@ public interface MetaStorageManager extends 
IgniteComponent {
  * Starts all registered watches.
  *
  * Should be called after all Ignite components have registered 
required watches and they are ready to process Meta Storage events.
+ *
+ * @return Future to complete.

Review Comment:
   Please add meaningful comment here.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228276622


##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();
 
 // Bump Meta Storage applied revision by modifying a fake key. 
DistributionZoneManager breaks on start if Vault is not empty, but
 // Meta Storage revision is equal to 0.
 var fakeKey = new ByteArray("foobar");
 
-CompletableFuture invokeFuture = metaStorageManager.invoke(
+CompletableFuture invokeFuture = 
deployWatchesFut.thenCompose(unused -> metaStorageManager.invoke(

Review Comment:
   Is it the only place we should await deployWatches? 



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228286531


##
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/impl/MetaStorageDeployWatchesCorrectnessTest.java:
##
@@ -0,0 +1,95 @@
+/*
+ * 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.metastorage.impl;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Set;
+import 
org.apache.ignite.internal.cluster.management.ClusterManagementGroupManager;
+import 
org.apache.ignite.internal.cluster.management.topology.api.LogicalTopologyService;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import 
org.apache.ignite.internal.metastorage.server.SimpleInMemoryKeyValueStorage;
+import org.apache.ignite.internal.raft.RaftManager;
+import org.apache.ignite.internal.testframework.IgniteAbstractTest;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.internal.vault.inmemory.InMemoryVaultService;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.apache.ignite.network.ClusterService;
+import org.junit.jupiter.api.Test;
+
+/**
+ * There are tests of correctness invocation {@link 
MetaStorageManager#deployWatches()}.

Review Comment:
   It's just "Test that checks correctness of an invocation ..." without "There 
are".



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] tkalkirill merged pull request #2184: IGNITE-19483 Transform TableManager and IndexManager to internally work against Catalog event types

[via GitHub]

tkalkirill merged PR #2184:
URL: https://github.com/apache/ignite-3/pull/2184


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228289667


##
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/impl/MetaStorageDeployWatchesCorrectnessTest.java:
##
@@ -0,0 +1,95 @@
+/*
+ * 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.metastorage.impl;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Set;
+import 
org.apache.ignite.internal.cluster.management.ClusterManagementGroupManager;
+import 
org.apache.ignite.internal.cluster.management.topology.api.LogicalTopologyService;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import 
org.apache.ignite.internal.metastorage.server.SimpleInMemoryKeyValueStorage;
+import org.apache.ignite.internal.raft.RaftManager;
+import org.apache.ignite.internal.testframework.IgniteAbstractTest;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.internal.vault.inmemory.InMemoryVaultService;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.apache.ignite.network.ClusterService;
+import org.junit.jupiter.api.Test;
+
+/**
+ * There are tests of correctness invocation {@link 
MetaStorageManager#deployWatches()}.
+ */
+public class MetaStorageDeployWatchesCorrectnessTest extends 
IgniteAbstractTest {
+@Test
+public void tesMetaStorageManager() throws Exception {
+HybridClock clock = new HybridClockImpl();
+String mcNodeName = "mc-node-1";
+
+ClusterManagementGroupManager cmgManager = 
mock(ClusterManagementGroupManager.class);
+ClusterService clusterService = mock(ClusterService.class);
+LogicalTopologyService logicalTopologyService = 
mock(LogicalTopologyService.class);
+RaftManager raftManager = mock(RaftManager.class);
+
+
when(cmgManager.metaStorageNodes()).thenReturn(completedFuture(Set.of(mcNodeName)));
+when(clusterService.nodeName()).thenReturn(mcNodeName);
+when(raftManager.startRaftGroupNodeAndWaitNodeReadyFuture(any(), 
any(), any(), any(), any())).thenReturn(completedFuture(null));
+
+var metastore = new MetaStorageManagerImpl(
+new VaultManager(new InMemoryVaultService()),

Review Comment:
   VaultManager is an Ignite service, so please either start/stop it or use 
mock instead.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228294924


##
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/impl/MetaStorageDeployWatchesCorrectnessTest.java:
##
@@ -0,0 +1,95 @@
+/*
+ * 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.metastorage.impl;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Set;
+import 
org.apache.ignite.internal.cluster.management.ClusterManagementGroupManager;
+import 
org.apache.ignite.internal.cluster.management.topology.api.LogicalTopologyService;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import 
org.apache.ignite.internal.metastorage.server.SimpleInMemoryKeyValueStorage;
+import org.apache.ignite.internal.raft.RaftManager;
+import org.apache.ignite.internal.testframework.IgniteAbstractTest;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.internal.vault.inmemory.InMemoryVaultService;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.apache.ignite.network.ClusterService;
+import org.junit.jupiter.api.Test;
+
+/**
+ * There are tests of correctness invocation {@link 
MetaStorageManager#deployWatches()}.
+ */
+public class MetaStorageDeployWatchesCorrectnessTest extends 
IgniteAbstractTest {
+@Test
+public void tesMetaStorageManager() throws Exception {
+HybridClock clock = new HybridClockImpl();
+String mcNodeName = "mc-node-1";
+
+ClusterManagementGroupManager cmgManager = 
mock(ClusterManagementGroupManager.class);
+ClusterService clusterService = mock(ClusterService.class);
+LogicalTopologyService logicalTopologyService = 
mock(LogicalTopologyService.class);
+RaftManager raftManager = mock(RaftManager.class);
+
+
when(cmgManager.metaStorageNodes()).thenReturn(completedFuture(Set.of(mcNodeName)));
+when(clusterService.nodeName()).thenReturn(mcNodeName);
+when(raftManager.startRaftGroupNodeAndWaitNodeReadyFuture(any(), 
any(), any(), any(), any())).thenReturn(completedFuture(null));
+
+var metastore = new MetaStorageManagerImpl(
+new VaultManager(new InMemoryVaultService()),
+clusterService,
+cmgManager,
+logicalTopologyService,
+raftManager,
+new SimpleInMemoryKeyValueStorage(mcNodeName),
+clock
+);
+
+checkCorrectness(metastore);

Review Comment:
   I'd rather use parametrized test here with method source (MethodSource) that 
will either return StandaloneMetaStorageManager or General meta storage. In 
that case checkCorrectness should be refactored to parametrized test itself.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] lowka commented on a diff in pull request #2140: [IGNITE-19587] Sql. Remove execution-related part from IgniteTable.

[via GitHub]

lowka commented on code in PR #2140:
URL: https://github.com/apache/ignite-3/pull/2140#discussion_r1228296263


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/QueryTaskExecutor.java:
##
@@ -45,4 +45,13 @@ public interface QueryTaskExecutor extends LifecycleAware, 
Executor {
  * @return the new CompletableFuture
  */
 CompletableFuture submit(UUID qryId, long fragmentId, Runnable qryTask);
+
+/**
+ * Returns an executor that is responsible for the given query fragment.
+ *
+ * @param qryId  Query ID.
+ * @param fragmentId Fragment ID.
+ * @return  An executor.
+ */
+Executor fragmentExecutor(UUID qryId, long fragmentId);

Review Comment:
   Done.



##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##
@@ -114,6 +116,9 @@ public class SqlQueryProcessor implements QueryProcessor {
 /** Size of the cache for query plans. */
 private static final int PLAN_CACHE_SIZE = 1024;
 
+/** Size of the table access cache. */
+public static final int TABLE_CACHE_SIZE = 1024;

Review Comment:
   Fixed.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] lowka commented on a diff in pull request #2140: [IGNITE-19587] Sql. Remove execution-related part from IgniteTable.

[via GitHub]

lowka commented on code in PR #2140:
URL: https://github.com/apache/ignite-3/pull/2140#discussion_r1228296810


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutableTable.java:
##
@@ -0,0 +1,41 @@
+/*
+ * 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.sql.engine.exec;
+
+import org.apache.ignite.internal.table.InternalTable;
+
+/**
+ * Execution related APIs of a table.
+ */
+public interface ExecutableTable {
+
+/**
+ * Returns read API.
+ */
+InternalTable table();

Review Comment:
   Done.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228302585


##
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/impl/MetaStorageDeployWatchesCorrectnessTest.java:
##
@@ -0,0 +1,95 @@
+/*
+ * 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.metastorage.impl;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Set;
+import 
org.apache.ignite.internal.cluster.management.ClusterManagementGroupManager;
+import 
org.apache.ignite.internal.cluster.management.topology.api.LogicalTopologyService;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import 
org.apache.ignite.internal.metastorage.server.SimpleInMemoryKeyValueStorage;
+import org.apache.ignite.internal.raft.RaftManager;
+import org.apache.ignite.internal.testframework.IgniteAbstractTest;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.internal.vault.inmemory.InMemoryVaultService;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.apache.ignite.network.ClusterService;
+import org.junit.jupiter.api.Test;
+
+/**
+ * There are tests of correctness invocation {@link 
MetaStorageManager#deployWatches()}.
+ */
+public class MetaStorageDeployWatchesCorrectnessTest extends 
IgniteAbstractTest {
+@Test
+public void tesMetaStorageManager() throws Exception {
+HybridClock clock = new HybridClockImpl();
+String mcNodeName = "mc-node-1";
+
+ClusterManagementGroupManager cmgManager = 
mock(ClusterManagementGroupManager.class);
+ClusterService clusterService = mock(ClusterService.class);
+LogicalTopologyService logicalTopologyService = 
mock(LogicalTopologyService.class);
+RaftManager raftManager = mock(RaftManager.class);
+
+
when(cmgManager.metaStorageNodes()).thenReturn(completedFuture(Set.of(mcNodeName)));
+when(clusterService.nodeName()).thenReturn(mcNodeName);
+when(raftManager.startRaftGroupNodeAndWaitNodeReadyFuture(any(), 
any(), any(), any(), any())).thenReturn(completedFuture(null));
+
+var metastore = new MetaStorageManagerImpl(
+new VaultManager(new InMemoryVaultService()),
+clusterService,
+cmgManager,
+logicalTopologyService,
+raftManager,
+new SimpleInMemoryKeyValueStorage(mcNodeName),
+clock
+);
+
+checkCorrectness(metastore);
+}
+
+@Test
+public void tesStandaloneMetaStorageManager() throws Exception {
+var metastore = StandaloneMetaStorageManager.create(new 
VaultManager(new InMemoryVaultService()));
+
+checkCorrectness(metastore);
+}
+
+/**
+ * Invokes {@link MetaStorageManager#deployWatches()} and checks result.
+ *
+ * @param metastore Meta storage.
+ * @throws NodeStoppingException If failed.
+ */
+private static void checkCorrectness(MetaStorageManager metastore) throws 
NodeStoppingException {
+var deployWatchesFut = metastore.deployWatches();
+
+assertFalse(deployWatchesFut.isDone());
+
+metastore.start();

Review Comment:
metastore.start() is  async thus `deployWatchesFut.isDone()` isn't 
guaranteed without some awaiting logic. Please use 
   
`org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher#willSucceedFast`
 or similar instead.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] vldpyatkov commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

vldpyatkov commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228308032


##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();

Review Comment:
   This is a test classe. I think it is possible for test purposes.



##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();

Review Comment:
   This is a test class. I think it is possible for test purposes.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] vldpyatkov commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

vldpyatkov commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228313489


##
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/impl/MetaStorageDeployWatchesCorrectnessTest.java:
##
@@ -0,0 +1,95 @@
+/*
+ * 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.metastorage.impl;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Set;
+import 
org.apache.ignite.internal.cluster.management.ClusterManagementGroupManager;
+import 
org.apache.ignite.internal.cluster.management.topology.api.LogicalTopologyService;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import 
org.apache.ignite.internal.metastorage.server.SimpleInMemoryKeyValueStorage;
+import org.apache.ignite.internal.raft.RaftManager;
+import org.apache.ignite.internal.testframework.IgniteAbstractTest;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.internal.vault.inmemory.InMemoryVaultService;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.apache.ignite.network.ClusterService;
+import org.junit.jupiter.api.Test;
+
+/**
+ * There are tests of correctness invocation {@link 
MetaStorageManager#deployWatches()}.
+ */
+public class MetaStorageDeployWatchesCorrectnessTest extends 
IgniteAbstractTest {
+@Test
+public void tesMetaStorageManager() throws Exception {
+HybridClock clock = new HybridClockImpl();
+String mcNodeName = "mc-node-1";
+
+ClusterManagementGroupManager cmgManager = 
mock(ClusterManagementGroupManager.class);
+ClusterService clusterService = mock(ClusterService.class);
+LogicalTopologyService logicalTopologyService = 
mock(LogicalTopologyService.class);
+RaftManager raftManager = mock(RaftManager.class);
+
+
when(cmgManager.metaStorageNodes()).thenReturn(completedFuture(Set.of(mcNodeName)));
+when(clusterService.nodeName()).thenReturn(mcNodeName);
+when(raftManager.startRaftGroupNodeAndWaitNodeReadyFuture(any(), 
any(), any(), any(), any())).thenReturn(completedFuture(null));
+
+var metastore = new MetaStorageManagerImpl(
+new VaultManager(new InMemoryVaultService()),
+clusterService,
+cmgManager,
+logicalTopologyService,
+raftManager,
+new SimpleInMemoryKeyValueStorage(mcNodeName),
+clock
+);
+
+checkCorrectness(metastore);
+}
+
+@Test
+public void tesStandaloneMetaStorageManager() throws Exception {
+var metastore = StandaloneMetaStorageManager.create(new 
VaultManager(new InMemoryVaultService()));
+
+checkCorrectness(metastore);
+}
+
+/**
+ * Invokes {@link MetaStorageManager#deployWatches()} and checks result.
+ *
+ * @param metastore Meta storage.
+ * @throws NodeStoppingException If failed.
+ */
+private static void checkCorrectness(MetaStorageManager metastore) throws 
NodeStoppingException {
+var deployWatchesFut = metastore.deployWatches();
+
+assertFalse(deployWatchesFut.isDone());
+
+metastore.start();

Review Comment:
   It is just a unit test, I know the behavior here.
   I am sure we will invoke the listener in the same thread where the start 
future is completed. 



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] vldpyatkov commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

vldpyatkov commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228322151


##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();
 
 // Bump Meta Storage applied revision by modifying a fake key. 
DistributionZoneManager breaks on start if Vault is not empty, but
 // Meta Storage revision is equal to 0.
 var fakeKey = new ByteArray("foobar");
 
-CompletableFuture invokeFuture = metaStorageManager.invoke(
+CompletableFuture invokeFuture = 
deployWatchesFut.thenCompose(unused -> metaStorageManager.invoke(

Review Comment:
   I do not know. I add explicit waiting here because it is a common test 
class, which might be used in different circumstances.
   Other places probably have internal guaranties or should wait to this future.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sk0x50 merged pull request #2171: IGNITE-19600 Removed topologyVersionedDataNodes

[via GitHub]

sk0x50 merged PR #2171:
URL: https://github.com/apache/ignite-3/pull/2171


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] denis-chudov closed pull request #2179: IGNITE-19079 ExecutionTimeout in ItIgniteNodeRestartTest

[via GitHub]

denis-chudov closed pull request #2179: IGNITE-19079 ExecutionTimeout in 
ItIgniteNodeRestartTest
URL: https://github.com/apache/ignite-3/pull/2179


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] denis-chudov opened a new pull request, #2187: IGNITE-19079 Enabled tests in ItIgniteNodeRestartTest: testTwoNodesRestartDirect and testTwoNodesRestartReverse

[via GitHub]

denis-chudov opened a new pull request, #2187:
URL: https://github.com/apache/ignite-3/pull/2187

   https://issues.apache.org/jira/browse/IGNITE-19079


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] vldpyatkov commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

vldpyatkov commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228354015


##
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/MetaStorageManager.java:
##
@@ -170,8 +170,10 @@ public interface MetaStorageManager extends 
IgniteComponent {
  * Starts all registered watches.
  *
  * Should be called after all Ignite components have registered 
required watches and they are ready to process Meta Storage events.
+ *
+ * @return Future to complete.

Review Comment:
   Could you provide something what you want to see here?



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228359724


##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();

Review Comment:
   > I think it is possible for test purposes.
   I've never heard about such possibilities for test classes. Is it allowed by 
our coding conventions?



##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();

Review Comment:
   > I think it is possible for test purposes.
   
   I've never heard about such possibilities for test classes. Is it allowed by 
our coding conventions?



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228365449


##
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/MetaStorageManager.java:
##
@@ -170,8 +170,10 @@ public interface MetaStorageManager extends 
IgniteComponent {
  * Starts all registered watches.
  *
  * Should be called after all Ignite components have registered 
required watches and they are ready to process Meta Storage events.
+ *
+ * @return Future to complete.

Review Comment:
   Something like: "future that'll completed when meta storage manager is 
started and deploying watches is finished."



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228369000


##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();
 
 // Bump Meta Storage applied revision by modifying a fake key. 
DistributionZoneManager breaks on start if Vault is not empty, but
 // Meta Storage revision is equal to 0.
 var fakeKey = new ByteArray("foobar");
 
-CompletableFuture invokeFuture = metaStorageManager.invoke(
+CompletableFuture invokeFuture = 
deployWatchesFut.thenCompose(unused -> metaStorageManager.invoke(

Review Comment:
   Well, seems that you've substituted sync method with an async one and thus 
it's your aim to check whether you break the guaranties in any other places and 
add chaining or join wherever needed (if needed).



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] zstan commented on a diff in pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping

[via GitHub]

zstan commented on code in PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#discussion_r1228370294


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java:
##
@@ -85,33 +231,19 @@ public MetadataDef getDef() {
  * @param mq  Metadata query instance. Used to request appropriate 
metadata from node children.
  * @return Nodes mapping, representing a list of nodes capable to execute 
a query over particular partitions.
  */
-public FragmentMapping fragmentMapping(RelNode rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
-throw new AssertionError();
-}
-
-/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
- */
-public FragmentMapping fragmentMapping(RelSubset rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
-throw new AssertionError();
-}
-
-/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
- */
-public FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
+private static FragmentMapping fragmentMapping(SingleRel rel, 
RelMetadataQuery mq, MappingQueryContext ctx) {
 return fragmentMappingForMetadataQuery(rel.getInput(), mq, ctx);
 }
 
 /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
+ * See {@link IgniteFragmentMapping#fragmentMapping(SingleRel, 
RelMetadataQuery, MappingQueryContext)}.
  *
  * {@link ColocationMappingException} may be thrown on two children 
nodes locations merge. This means that the fragment
  * (which part the parent node is) cannot be executed on any node and 
additional exchange is needed. This case we throw {@link
  * NodeMappingException} with an edge, where we need the additional 
exchange. After the exchange is put into the fragment and the
  * fragment is split into two ones, fragment meta information will be 
recalculated for all fragments.
  */
-public FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
+private static FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery 
mq, MappingQueryContext ctx) {

Review Comment:
   we can inline them only partially, for example 
   `fragmentMapping(BiRel rel` it\`s about CorrJoin, NestedJoin and LoopJoin too



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

sanpwc commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228373071


##
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/impl/MetaStorageDeployWatchesCorrectnessTest.java:
##
@@ -0,0 +1,95 @@
+/*
+ * 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.metastorage.impl;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Set;
+import 
org.apache.ignite.internal.cluster.management.ClusterManagementGroupManager;
+import 
org.apache.ignite.internal.cluster.management.topology.api.LogicalTopologyService;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import 
org.apache.ignite.internal.metastorage.server.SimpleInMemoryKeyValueStorage;
+import org.apache.ignite.internal.raft.RaftManager;
+import org.apache.ignite.internal.testframework.IgniteAbstractTest;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.internal.vault.inmemory.InMemoryVaultService;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.apache.ignite.network.ClusterService;
+import org.junit.jupiter.api.Test;
+
+/**
+ * There are tests of correctness invocation {@link 
MetaStorageManager#deployWatches()}.
+ */
+public class MetaStorageDeployWatchesCorrectnessTest extends 
IgniteAbstractTest {
+@Test
+public void tesMetaStorageManager() throws Exception {
+HybridClock clock = new HybridClockImpl();
+String mcNodeName = "mc-node-1";
+
+ClusterManagementGroupManager cmgManager = 
mock(ClusterManagementGroupManager.class);
+ClusterService clusterService = mock(ClusterService.class);
+LogicalTopologyService logicalTopologyService = 
mock(LogicalTopologyService.class);
+RaftManager raftManager = mock(RaftManager.class);
+
+
when(cmgManager.metaStorageNodes()).thenReturn(completedFuture(Set.of(mcNodeName)));
+when(clusterService.nodeName()).thenReturn(mcNodeName);
+when(raftManager.startRaftGroupNodeAndWaitNodeReadyFuture(any(), 
any(), any(), any(), any())).thenReturn(completedFuture(null));
+
+var metastore = new MetaStorageManagerImpl(
+new VaultManager(new InMemoryVaultService()),
+clusterService,
+cmgManager,
+logicalTopologyService,
+raftManager,
+new SimpleInMemoryKeyValueStorage(mcNodeName),
+clock
+);
+
+checkCorrectness(metastore);
+}
+
+@Test
+public void tesStandaloneMetaStorageManager() throws Exception {
+var metastore = StandaloneMetaStorageManager.create(new 
VaultManager(new InMemoryVaultService()));
+
+checkCorrectness(metastore);
+}
+
+/**
+ * Invokes {@link MetaStorageManager#deployWatches()} and checks result.
+ *
+ * @param metastore Meta storage.
+ * @throws NodeStoppingException If failed.
+ */
+private static void checkCorrectness(MetaStorageManager metastore) throws 
NodeStoppingException {
+var deployWatchesFut = metastore.deployWatches();
+
+assertFalse(deployWatchesFut.isDone());
+
+metastore.start();

Review Comment:
   Why should we exposes such implementation details (e.g. the one that there's 
no executors or similar inside metaStorage.start) instead of writing guaranteed 
code based on willSucceedFast?



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] zstan commented on a diff in pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping

[via GitHub]

zstan commented on code in PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#discussion_r1228370294


##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java:
##
@@ -85,33 +231,19 @@ public MetadataDef getDef() {
  * @param mq  Metadata query instance. Used to request appropriate 
metadata from node children.
  * @return Nodes mapping, representing a list of nodes capable to execute 
a query over particular partitions.
  */
-public FragmentMapping fragmentMapping(RelNode rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
-throw new AssertionError();
-}
-
-/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
- */
-public FragmentMapping fragmentMapping(RelSubset rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
-throw new AssertionError();
-}
-
-/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
- */
-public FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
+private static FragmentMapping fragmentMapping(SingleRel rel, 
RelMetadataQuery mq, MappingQueryContext ctx) {
 return fragmentMappingForMetadataQuery(rel.getInput(), mq, ctx);
 }
 
 /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, 
RelMetadataQuery, MappingQueryContext)}.
+ * See {@link IgniteFragmentMapping#fragmentMapping(SingleRel, 
RelMetadataQuery, MappingQueryContext)}.
  *
  * {@link ColocationMappingException} may be thrown on two children 
nodes locations merge. This means that the fragment
  * (which part the parent node is) cannot be executed on any node and 
additional exchange is needed. This case we throw {@link
  * NodeMappingException} with an edge, where we need the additional 
exchange. After the exchange is put into the fragment and the
  * fragment is split into two ones, fragment meta information will be 
recalculated for all fragments.
  */
-public FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, 
MappingQueryContext ctx) {
+private static FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery 
mq, MappingQueryContext ctx) {

Review Comment:
   we can inline them only partially, for example 
   `fragmentMapping(BiRel rel` it\`s about CorrJoin, NestedJoin and MergeJoin 
too



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] vldpyatkov commented on a diff in pull request #2183: IGNITE-19606 Linearize metaStorageManager.deployWatches and metaStora…

[via GitHub]

vldpyatkov commented on code in PR #2183:
URL: https://github.com/apache/ignite-3/pull/2183#discussion_r1228652145


##
modules/distribution-zones/src/testFixtures/java/org/apache/ignite/internal/distributionzones/DistributionZonesTestUtil.java:
##
@@ -313,17 +313,17 @@ public static void 
deployWatchesAndUpdateMetaStorageRevision(MetaStorageManager
 throws NodeStoppingException, InterruptedException {
 // Watches are deployed before distributionZoneManager start in order 
to update Meta Storage revision before
 // distributionZoneManager's recovery.
-metaStorageManager.deployWatches();
+var deployWatchesFut = metaStorageManager.deployWatches();
 
 // Bump Meta Storage applied revision by modifying a fake key. 
DistributionZoneManager breaks on start if Vault is not empty, but
 // Meta Storage revision is equal to 0.
 var fakeKey = new ByteArray("foobar");
 
-CompletableFuture invokeFuture = metaStorageManager.invoke(
+CompletableFuture invokeFuture = 
deployWatchesFut.thenCompose(unused -> metaStorageManager.invoke(

Review Comment:
   I think, if before, we have no guarantees that MC start earlier than watches 
subscribe, it means thus tests are buggy.
   Ok, add synchronous waiting in every invocation.



-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] ptupitsyn opened a new pull request, #2189: IGNITE-19728 .NET: Fix TestAutoFlushFrequency flakiness

[via GitHub]

ptupitsyn opened a new pull request, #2189:
URL: https://github.com/apache/ignite-3/pull/2189

   (no comment)


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite-3] zstan opened a new pull request, #2190: IGNITE-19373 Get rid of waitForIndex from tests

[via GitHub]

zstan opened a new pull request, #2190:
URL: https://github.com/apache/ignite-3/pull/2190

   (no comment)


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [ignite] sonarcloud[bot] commented on pull request #10770: IGNITE-19715 Thin client socket#open timeout changed to ClientConfiguration#timeout property.

[via GitHub]

sonarcloud[bot] commented on PR #10770:
URL: https://github.com/apache/ignite/pull/10770#issuecomment-1590594553

   Kudos, SonarCloud Quality Gate passed!    [![Quality Gate 
passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png
 'Quality Gate 
passed')](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=10770)
   
   
[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png
 
'Bug')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=BUG)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=BUG)
 [0 
Bugs](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=BUG)
  
   
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png
 
'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=VULNERABILITY)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=VULNERABILITY)
 [0 
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=VULNERABILITY)
  
   [![Security 
Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png
 'Security 
Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10770&resolved=false&types=SECURITY_HOTSPOT)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10770&resolved=false&types=SECURITY_HOTSPOT)
 [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_ignite&pullRequest=10770&resolved=false&types=SECURITY_HOTSPOT)
  
   [![Code 
Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png
 'Code 
Smell')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=CODE_SMELL)
 
[![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png
 
'A')](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=CODE_SMELL)
 [0 Code 
Smells](https://sonarcloud.io/project/issues?id=apache_ignite&pullRequest=10770&resolved=false&types=CODE_SMELL)
   
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10770&metric=new_coverage&view=list)
 [0.0% 
Coverage](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10770&metric=new_coverage&view=list)
  
   
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png
 
'0.0%')](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10770&metric=new_duplicated_lines_density&view=list)
 [0.0% 
Duplication](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=10770&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: notifications-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org