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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
