-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
messagechannel.send() implementation #102
Conversation
Test Results213 tests 213 ✅ 34s ⏱️ Results for commit 1f43d2e. ♻️ This comment has been updated with latest results. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #102 +/- ##
============================================
- Coverage 72.37% 72.08% -0.30%
- Complexity 502 510 +8
============================================
Files 101 103 +2
Lines 2201 2246 +45
Branches 135 138 +3
============================================
+ Hits 1593 1619 +26
- Misses 555 572 +17
- Partials 53 55 +2 ☔ View full report in Codecov by Sentry. |
core/src/main/java/com/flipkart/varadhi/core/cluster/ClusterMessage.java
Outdated
Show resolved
Hide resolved
void addMessageHandler(String path, MessageHandler messageHandler); | ||
|
||
void removeMessageHandler(String path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add / remove methods dont look like they belong here. They can only remain in Impl class I think.
<E extends ClusterMessage> CompletableFuture<Void> handle(E message); | ||
|
||
// request will be used for messages sent via channel.request(). | ||
<E extends ClusterMessage> CompletableFuture<ResponseMessage> request(E message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<E extends ClusterMessage> CompletableFuture<ResponseMessage> request(E message); | |
<E extends ClusterMessage> CompletableFuture<ResponseMessage> handleRequest(E message); |
Does the params need sender info? E message might not have sender info.
log.info("publish({}) message processed.", clusterMessage.getId()); | ||
} else { | ||
String deliveryKind = message.headers().get(DELIVERY_KIND_HEADER); | ||
if (DELIVERY_KIND_SEND.equals(deliveryKind)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the checks like "DELIVERY_KIND_SEND.equals(deliveryKind)" required?
Feels redundant. I thought that "expecting" response is kinda implicit due to the api.
For these things to work properly. the sender and reciever needs to be compatible. But having this header does not make it "correct" or enforce correctness.
For eg:
sender could have just "Send" but here we are registering a consumer which actually returns something. So does it mean that the "sender" establishes the expectation of "no response"? But consumer has a response so, is that correct?
The vice-versa is that consumer establishes expectation that there will be response and it is upto sender to ignore that or use that?
Frankly neither feels 100% correct. Which means that the correctness / compatibility enforcement is "implicit". If we get it wrong, well we should have tested it, but there is no way to enforce that in runtime. So the "deliveryKind" header effectively looks redundant as it is not bringing any more validations.
- message channel PR comments
Closes #94