You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "zstan (via GitHub)" <gi...@apache.org> on 2023/06/22 17:58:17 UTC
[GitHub] [ignite-3] zstan opened a new pull request, #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
zstan opened a new pull request, #2241:
URL: https://github.com/apache/ignite-3/pull/2241
(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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1246227747
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
it's not news that we don't have comprehensive test coverage.
At the very beginning, the comment states "Some fragments may be completed exceptionally". Therefore, we need to find out whether this invariant is conserved. There are only 5 usages `remoteFragmentInitCompletion`, so I believe it's not a rocket science to take a look at every usage. As far as I can see, `DistributedQueryManager#onNodeLeft` completes init future with exception.
Feel free to add such test to ExecutionServiceImplTest. If it will pass without mentioned `handle` block, I'm ok to 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1244838075
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
as i can see : 1. remoteFragmentInitCompletion holder completes it`s futures only on one place 2. i don`t understand what problem solve this code if future somethere completes exceptionally. Plz give me the clue
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1245063024
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
@@ -107,7 +105,7 @@ public void testSessionExpiration() throws Exception {
while (rs1.hasNext()) {
rs1.next();
}
- }, ExecutionCancelledException.class);
+ }, CursorClosedException.class);
Review Comment:
got it, thanks for pointing, bloody bloody code )
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1250363722
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -838,33 +838,38 @@ private CompletableFuture<Void> close(boolean cancel) {
: closeLocalFragments();
var finalStepFut = cancelResult.whenComplete((r, e) -> {
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
+
+ LOG.warn(format("Fragment closing processed with errors, root fragmentId={}", rootFragmentId), ex);
Review Comment:
I've got a few questions regarding this line of code:
1) why don't delegate formatting to logger?
2) how did you come up with rootFragmentId param?
3) have you read logging guidelines?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1252208917
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -838,33 +838,38 @@ private CompletableFuture<Void> close(boolean cancel) {
: closeLocalFragments();
var finalStepFut = cancelResult.whenComplete((r, e) -> {
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
+
+ LOG.warn("Fragment closing processed with errors, root [queryId={}]", ex, ctx.queryId());
Review Comment:
it`s all about "self review" )
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1244714682
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java:
##########
@@ -690,7 +689,7 @@ public void closeSession() {
assertThrowsWithCause(
() -> await(ars0.fetchNextPage()),
- ExecutionCancelledException.class
+ CursorClosedException.class
Review Comment:
the same
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/StopCalciteModuleTest.java:
##########
@@ -284,7 +284,7 @@ public void testStopQueryOnNodeStop() throws Exception {
await(request.exceptionally(t -> {
assertInstanceOf(CompletionException.class, t);
assertInstanceOf(IgniteException.class, t.getCause());
- assertInstanceOf(ExecutionCancelledException.class, t.getCause().getCause());
+ assertInstanceOf(CursorClosedException.class, t.getCause().getCause());
Review Comment:
and here
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -824,47 +824,62 @@ private void enlist(SourceAwareIgniteRel rel) {
}.visit(fragment.root());
}
- private CompletableFuture<Void> close(boolean cancel) {
+ private CompletableFuture<Void> close() {
if (!cancelled.compareAndSet(false, true)) {
- return cancelFut.thenApply(Function.identity());
+ return cancelFut;
}
- CompletableFuture<Void> start = closeExecNode(cancel);
+ CompletableFuture<Void> start = closeExecNode();
start
- .thenCompose(tmp -> {
- CompletableFuture<Void> cancelResult = coordinator
- ? awaitFragmentInitialisationAndClose()
- : closeLocalFragments();
-
- var finalStepFut = cancelResult.whenComplete((r, e) -> {
- queryManagerMap.remove(ctx.queryId());
-
- try {
- ctx.cancel().cancel();
- } catch (Exception ignored) {
- // NO-OP
- }
+ .thenCompose(tmp -> {
Review Comment:
indentation
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
why did you remove this?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
@@ -107,7 +105,7 @@ public void testSessionExpiration() throws Exception {
while (rs1.hasNext()) {
rs1.next();
}
- }, ExecutionCancelledException.class);
+ }, CursorClosedException.class);
Review Comment:
well, I think this change is not correct. In my opinion, we need to distinguish between cursor closed by users' intention and cursor closed by system intention
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1244838075
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
as i can see : 1. remoteFragmentInitCompletion holder completes it\`s futures only on one place 2. i don\`t understand what problem solve this code if future somethere completes exceptionally. Plz give me the clue
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1252124233
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -838,33 +838,38 @@ private CompletableFuture<Void> close(boolean cancel) {
: closeLocalFragments();
var finalStepFut = cancelResult.whenComplete((r, e) -> {
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
+
+ LOG.warn("Fragment closing processed with errors, root [queryId={}]", ex, ctx.queryId());
Review Comment:
not sure why do we need "root" at the end
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1245052369
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
have you read comment in this empty block?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1245065903
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
of course, but still have no clue, i found that no one test will touch this place with not null **t**
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1247506243
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -838,33 +838,34 @@ private CompletableFuture<Void> close(boolean cancel) {
: closeLocalFragments();
var finalStepFut = cancelResult.whenComplete((r, e) -> {
- queryManagerMap.remove(ctx.queryId());
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
- try {
- ctx.cancel().cancel();
- } catch (Exception ignored) {
- // NO-OP
+ LOG.warn("Exception raised during close", ex);
}
+ queryManagerMap.remove(ctx.queryId());
+
+ ctx.cancel().cancel();
Review Comment:
why did you removed try-catch block around cancel?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1246212126
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -826,45 +826,69 @@ private void enlist(SourceAwareIgniteRel rel) {
private CompletableFuture<Void> close(boolean cancel) {
if (!cancelled.compareAndSet(false, true)) {
- return cancelFut.thenApply(Function.identity());
+ return cancelFut;
}
CompletableFuture<Void> start = closeExecNode(cancel);
- start
- .thenCompose(tmp -> {
- CompletableFuture<Void> cancelResult = coordinator
- ? awaitFragmentInitialisationAndClose()
- : closeLocalFragments();
+ start.thenCompose(tmp -> {
+ CompletableFuture<Void> cancelResult = coordinator
+ ? awaitFragmentInitialisationAndClose()
+ : closeLocalFragments();
- var finalStepFut = cancelResult.whenComplete((r, e) -> {
- queryManagerMap.remove(ctx.queryId());
+ var finalStepFut = cancelResult.whenComplete((r, e) -> {
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
- try {
- ctx.cancel().cancel();
- } catch (Exception ignored) {
- // NO-OP
- }
+ LOG.warn("Exception raised during close", ex);
+ }
- cancelFut.complete(null);
- });
+ queryManagerMap.remove(ctx.queryId());
- return cancelResult.thenCombine(finalStepFut, (none1, none2) -> null);
- });
+ ctx.cancel().cancel();
+
+ cancelFut.complete(null);
+ });
+
+ return cancelResult.thenCombine(finalStepFut, (none1, none2) -> null);
+ })
+ .thenRun(() -> localFragments.forEach(f -> f.context().cancel()));
start.completeAsync(() -> null, taskExecutor);
- return cancelFut.thenApply(Function.identity());
+ return cancelFut;
+ }
+
+ /**
+ * Synchronously closes the tree's execution iterator.
+ *
+ * @param cancel Forces execution to terminate with {@link ExecutionCancelledException}.
+ * @return Completable future that should run asynchronously.
+ */
+ private CompletableFuture<Void> closeExecNode(boolean cancel) {
+ CompletableFuture<Void> start = new CompletableFuture<>();
+
+ if (!root.completeExceptionally(new ExecutionCancelledException()) && !root.isCompletedExceptionally()) {
+ AsyncRootNode<RowT, List<Object>> node = root.getNow(null);
+
+ if (!cancel) {
+ CompletableFuture<Void> closeFut = node.closeAsync();
+
+ return start.thenCompose(v -> closeFut);
+ }
+
+ node.onError(new ExecutionCancelledException());
+ }
+
+ return start;
Review Comment:
is there any reason to move this method up? If no, let's just revert this change
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1247840507
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -838,33 +838,34 @@ private CompletableFuture<Void> close(boolean cancel) {
: closeLocalFragments();
var finalStepFut = cancelResult.whenComplete((r, e) -> {
- queryManagerMap.remove(ctx.queryId());
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
- try {
- ctx.cancel().cancel();
- } catch (Exception ignored) {
- // NO-OP
+ LOG.warn("Exception raised during close", ex);
}
+ queryManagerMap.remove(ctx.queryId());
+
+ ctx.cancel().cancel();
Review Comment:
revert and fill the issue https://issues.apache.org/jira/browse/IGNITE-19895
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#issuecomment-1613161296
@korlov42 return error handling, append test.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1250404649
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -838,33 +838,38 @@ private CompletableFuture<Void> close(boolean cancel) {
: closeLocalFragments();
var finalStepFut = cancelResult.whenComplete((r, e) -> {
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
+
+ LOG.warn(format("Fragment closing processed with errors, root fragmentId={}", rootFragmentId), ex);
Review Comment:
ok, i change 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1244802526
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java:
##########
@@ -690,7 +689,7 @@ public void closeSession() {
assertThrowsWithCause(
() -> await(ars0.fetchNextPage()),
- ExecutionCancelledException.class
+ CursorClosedException.class
Review Comment:
and what about already exist :
https://github.com/apache/ignite-3/blob/cae90410a32ba3946d09f6a135683f9c3e231f38/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java#L636
https://github.com/apache/ignite-3/blob/cae90410a32ba3946d09f6a135683f9c3e231f38/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java#L330
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#issuecomment-1609355234
@korlov42 tests are not flaky any more, as for me code become a little bit clear for understanding. can u make a review plz ?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1247515629
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -838,33 +838,34 @@ private CompletableFuture<Void> close(boolean cancel) {
: closeLocalFragments();
var finalStepFut = cancelResult.whenComplete((r, e) -> {
- queryManagerMap.remove(ctx.queryId());
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
- try {
- ctx.cancel().cancel();
- } catch (Exception ignored) {
- // NO-OP
+ LOG.warn("Exception raised during close", ex);
Review Comment:
Log message should be rephrased.
According to our [guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Logging+Guidelines#JavaLoggingGuidelines-WARN):
```
WARN is what the team will need to review at the start of the next business day, at a high priority. WARN is always actionable - the user must understand how to react
```
From the message I don't understand what I should to do. TBH, I don't even understand what was going wrong. We closed a door and exception had been raised? A window?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1244912561
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java:
##########
@@ -690,7 +689,7 @@ public void closeSession() {
assertThrowsWithCause(
() -> await(ars0.fetchNextPage()),
- ExecutionCancelledException.class
+ CursorClosedException.class
Review Comment:
what is wrong with these two assertion?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1244990085
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
))) as i can observe through history initially this section process exception handling and after just degenerate into empty block. I agree To fill the issue for this case if you ok.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1246227747
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
it's not news that we don't have comprehensive test coverage.
In the very beginning, the comment states "Some fragments may be completed exceptionally". Therefore, we need to find out whether this invariant is conserved. There are only 5 usages `remoteFragmentInitCompletion`, so I believe it's not a rocket science to take a look at every usage. As far as I can see, `DistributedQueryManager#onNodeLeft` completes init future with exception.
Feel free to add such test to ExecutionServiceImplTest. If it will pass without mentioned `handle` block, I'm ok to 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1244846036
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -888,14 +903,6 @@ private CompletableFuture<Void> awaitFragmentInitialisationAndClose() {
cancelFuts.add(
CompletableFuture.allOf(entry.getValue().toArray(new CompletableFuture[0]))
- .handle((none2, t) -> {
- // Some fragments may be completed exceptionally, and that is fine.
- // Here we need to make sure that all query initialisation requests
- // have been processed before sending a cancellation request. That's
- // why we just ignore any exception.
-
- return null;
- })
Review Comment:
https://en.wiktionary.org/wiki/Chesterton%27s_fence
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241#discussion_r1246229842
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -826,45 +826,69 @@ private void enlist(SourceAwareIgniteRel rel) {
private CompletableFuture<Void> close(boolean cancel) {
if (!cancelled.compareAndSet(false, true)) {
- return cancelFut.thenApply(Function.identity());
+ return cancelFut;
}
CompletableFuture<Void> start = closeExecNode(cancel);
- start
- .thenCompose(tmp -> {
- CompletableFuture<Void> cancelResult = coordinator
- ? awaitFragmentInitialisationAndClose()
- : closeLocalFragments();
+ start.thenCompose(tmp -> {
+ CompletableFuture<Void> cancelResult = coordinator
+ ? awaitFragmentInitialisationAndClose()
+ : closeLocalFragments();
- var finalStepFut = cancelResult.whenComplete((r, e) -> {
- queryManagerMap.remove(ctx.queryId());
+ var finalStepFut = cancelResult.whenComplete((r, e) -> {
+ if (e != null) {
+ Throwable ex = unwrapCause(e);
- try {
- ctx.cancel().cancel();
- } catch (Exception ignored) {
- // NO-OP
- }
+ LOG.warn("Exception raised during close", ex);
+ }
- cancelFut.complete(null);
- });
+ queryManagerMap.remove(ctx.queryId());
- return cancelResult.thenCombine(finalStepFut, (none1, none2) -> null);
- });
+ ctx.cancel().cancel();
+
+ cancelFut.complete(null);
+ });
+
+ return cancelResult.thenCombine(finalStepFut, (none1, none2) -> null);
+ })
+ .thenRun(() -> localFragments.forEach(f -> f.context().cancel()));
Review Comment:
any reason to reformat this part?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan merged pull request #2241: IGNITE-19730 Sql. ExecutionServiceImplTest different tests failed
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan merged PR #2241:
URL: https://github.com/apache/ignite-3/pull/2241
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org