You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "korlov42 (via GitHub)" <gi...@apache.org> on 2023/04/25 12:14:52 UTC
[GitHub] [ignite-3] korlov42 opened a new pull request, #1980: IGNITE-17685 Sql. SQL engine should send messages asynchronously
korlov42 opened a new pull request, #1980:
URL: https://github.com/apache/ignite-3/pull/1980
The patch is split onto two commits. The first one introduces dead code elimination and minor refactoring like moving sending of close message from ExchangeService. The second commit introduce actual refactoring of MessageService
--
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 #1980: IGNITE-17685 Sql. SQL engine should send messages asynchronously
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #1980:
URL: https://github.com/apache/ignite-3/pull/1980#discussion_r1178644520
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -467,15 +467,22 @@ private void sendFragment(
.schemaVersion(ctx.schemaVersion())
.build();
- var fut = new CompletableFuture<Void>();
- remoteFragmentInitCompletion.put(new RemoteFragmentKey(targetNodeName, fragment.fragmentId()), fut);
+ CompletableFuture<Void> remoteFragmentInitializationCompletionFuture = new CompletableFuture<>();
- try {
- messageService.send(targetNodeName, req);
- } catch (Exception ex) {
- fut.complete(null);
+ remoteFragmentInitCompletion.put(
+ new RemoteFragmentKey(targetNodeName, fragment.fragmentId()),
+ remoteFragmentInitializationCompletionFuture
+ );
- throw ex;
+ try {
+ return messageService.send(targetNodeName, request);
+ } catch (Throwable th) {
+ // it's not expected MessageService to throw any exception, yet it may be possible
Review Comment:
> additionally for now it throws NODE_LEFT_ERR
this is not how it expected to be, so thanks to pointing out!
> i think we don`t need to write about expectations
Although it's implementation defined, every implementation must conform the contract, which defined by MessageService interface. Currently, method `send` declares no exception in `throws` section. Besides, it returns future representing result of operation. Thus, I think it's ok in that particular case to expected of this method not to throw any exception.
Anyway, I reworked the test that make me put this try-catch block (and also fix missed NODE_LEFT_ERR), so we do not need it anymore.
--
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] AMashenkov commented on a diff in pull request #1980: IGNITE-17685 Sql. SQL engine should send messages asynchronously
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #1980:
URL: https://github.com/apache/ignite-3/pull/1980#discussion_r1177507705
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -467,15 +467,22 @@ private void sendFragment(
.schemaVersion(ctx.schemaVersion())
.build();
- var fut = new CompletableFuture<Void>();
- remoteFragmentInitCompletion.put(new RemoteFragmentKey(targetNodeName, fragment.fragmentId()), fut);
+ CompletableFuture<Void> remoteFragmentInitializationCompletionFuture = new CompletableFuture<>();
- try {
- messageService.send(targetNodeName, req);
- } catch (Exception ex) {
- fut.complete(null);
+ remoteFragmentInitCompletion.put(
+ new RemoteFragmentKey(targetNodeName, fragment.fragmentId()),
+ remoteFragmentInitializationCompletionFuture
+ );
- throw ex;
+ try {
+ return messageService.send(targetNodeName, request);
+ } catch (Throwable th) {
+ // it's not expected MessageService to throw any exception, yet it may be possible
+ // due to unpredictable side effects of different implementations. To avoid blocking
+ // of node on stopping, let's complete initialization future.
+ remoteFragmentInitializationCompletionFuture.complete(null);
Review Comment:
Should we also remove future from remoteFragmentInitCompletion map if exception is re-thrown.
--
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] AMashenkov commented on a diff in pull request #1980: IGNITE-17685 Sql. SQL engine should send messages asynchronously
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #1980:
URL: https://github.com/apache/ignite-3/pull/1980#discussion_r1177507705
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -467,15 +467,22 @@ private void sendFragment(
.schemaVersion(ctx.schemaVersion())
.build();
- var fut = new CompletableFuture<Void>();
- remoteFragmentInitCompletion.put(new RemoteFragmentKey(targetNodeName, fragment.fragmentId()), fut);
+ CompletableFuture<Void> remoteFragmentInitializationCompletionFuture = new CompletableFuture<>();
- try {
- messageService.send(targetNodeName, req);
- } catch (Exception ex) {
- fut.complete(null);
+ remoteFragmentInitCompletion.put(
+ new RemoteFragmentKey(targetNodeName, fragment.fragmentId()),
+ remoteFragmentInitializationCompletionFuture
+ );
- throw ex;
+ try {
+ return messageService.send(targetNodeName, request);
+ } catch (Throwable th) {
+ // it's not expected MessageService to throw any exception, yet it may be possible
+ // due to unpredictable side effects of different implementations. To avoid blocking
+ // of node on stopping, let's complete initialization future.
+ remoteFragmentInitializationCompletionFuture.complete(null);
Review Comment:
Should we also remove future from remoteFragmentInitCompletion map if exception is re-thrown.
--
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 merged pull request #1980: IGNITE-17685 Sql. SQL engine should send messages asynchronously
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 merged PR #1980:
URL: https://github.com/apache/ignite-3/pull/1980
--
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 #1980: IGNITE-17685 Sql. SQL engine should send messages asynchronously
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #1980:
URL: https://github.com/apache/ignite-3/pull/1980#discussion_r1177794311
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -467,15 +467,22 @@ private void sendFragment(
.schemaVersion(ctx.schemaVersion())
.build();
- var fut = new CompletableFuture<Void>();
- remoteFragmentInitCompletion.put(new RemoteFragmentKey(targetNodeName, fragment.fragmentId()), fut);
+ CompletableFuture<Void> remoteFragmentInitializationCompletionFuture = new CompletableFuture<>();
- try {
- messageService.send(targetNodeName, req);
- } catch (Exception ex) {
- fut.complete(null);
+ remoteFragmentInitCompletion.put(
+ new RemoteFragmentKey(targetNodeName, fragment.fragmentId()),
+ remoteFragmentInitializationCompletionFuture
+ );
- throw ex;
+ try {
+ return messageService.send(targetNodeName, request);
+ } catch (Throwable th) {
+ // it's not expected MessageService to throw any exception, yet it may be possible
Review Comment:
it confusing me, i think we don\`t need to write about expectations, as you mention - it\`s implementation dependent, additionally for now it throws NODE_LEFT_ERR. I suggest to change this comment.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -543,21 +550,19 @@ private void executeFragment(IgniteRel treeRoot, ExecutionContext<RowT> ectx) {
root.complete(rootNode);
}
- try {
- messageService.send(
- origNodeName,
- FACTORY.queryStartResponse()
- .queryId(ectx.queryId())
- .fragmentId(ectx.fragmentId())
- .build()
- );
- } catch (IgniteInternalCheckedException e) {
- throw new IgniteInternalException(MESSAGE_SEND_ERR, "Failed to send reply. [nodeName=" + origNodeName + ']', e);
Review Comment:
MESSAGE_SEND_ERR used only in tests for now
--
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 #1980: IGNITE-17685 Sql. SQL engine should send messages asynchronously
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #1980:
URL: https://github.com/apache/ignite-3/pull/1980#discussion_r1178636953
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -543,21 +550,19 @@ private void executeFragment(IgniteRel treeRoot, ExecutionContext<RowT> ectx) {
root.complete(rootNode);
}
- try {
- messageService.send(
- origNodeName,
- FACTORY.queryStartResponse()
- .queryId(ectx.queryId())
- .fragmentId(ectx.fragmentId())
- .build()
- );
- } catch (IgniteInternalCheckedException e) {
- throw new IgniteInternalException(MESSAGE_SEND_ERR, "Failed to send reply. [nodeName=" + origNodeName + ']', e);
Review Comment:
removed
--
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