IGNITE-17685 Sql. SQL engine should send messages asynchronously#1980
IGNITE-17685 Sql. SQL engine should send messages asynchronously#1980korlov42 merged 5 commits intoapache:mainfrom
Conversation
zstan
left a comment
zstan
left a comment
There was a problem hiding this comment.
looks good, besides minor comments
| try { | ||
| return messageService.send(targetNodeName, request); | ||
| } catch (Throwable th) { | ||
| // it's not expected MessageService to throw any exception, yet it may be possible |
There was a problem hiding this 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.
There was a problem hiding this 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.
...ql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java
Show resolved
Hide resolved
- MessageServiceImpl now is not throwing any exception - removed confusing try-catch - removed unused error code
4 participants
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