add initial message delivery implementation #151
Conversation
Test Results221 tests 221 ✅ 39s ⏱️ Results for commit b465499. ♻️ This comment has been updated with latest results. |
aayustark007-fk
force-pushed
the
message-delivery
branch
from
bf796b9
to
26ab682
Compare
some endpoints will return error code if GET or DELETE methods are called with a body Signed-off-by: aayustark007-fk <[email protected]>
| @@ -16,7 +17,8 @@ | |||
| }) | |||
| public abstract sealed class Endpoint { | |||
|
|
|||
| abstract Protocol getProtocol(); | |||
| @JsonIgnore | |||
| public abstract Protocol getProtocol(); | |||
There was a problem hiding this comment.
Instead of of using raw type name, we can actually use protocol string only in the JsonSubTypes annotation. It will make for nice json.
Eg:
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "protocol")
@JsonSubTypes({
@JsonSubTypes.Type(value = HttpEndpoint.class, name = "HTTP1_1"),
@JsonSubTypes.Type(value = HttpEndpoint.class, name = "HTTP2"),See if this works and would it result in a better serialized form?
There was a problem hiding this comment.
yes it works and this is the serialized json:
"endpoint" : {
"protocol" : "HTTP1_1",
"url" : "http://localhosthello",
"method" : "GET",
"contentType" : "",
"connectTimeoutMs" : 500,
"requestTimeoutMs" : 500,
"http2Supported" : false
}
|
|
||
| public HttpMessageDelivery(Endpoint endpoint) { | ||
| this.endpoint = (Endpoint.HttpEndpoint) endpoint; | ||
| this.httpClient = HttpClient.newBuilder() |
There was a problem hiding this comment.
We will likely not create a client per endpoint. There will be a single client used for all endpoints in an app.
Assume a ConsumerContext {
Threadpools..
eventloops....
HttpClient client,
... something related to grpc as well.
}
And expect the context to be passed to
MessageDelivery of(Endpoint endpoint) {
There was a problem hiding this comment.
So, can I model it as a Supplier<HttpClient> that needs to be passed, since the ConsumerContext does not exist yet
| private final Endpoint.HttpEndpoint endpoint; | ||
| private final HttpClient httpClient; | ||
|
|
||
| public HttpMessageDelivery(Endpoint endpoint) { |
There was a problem hiding this comment.
The constructor should take HttpEndpoint as param.
|
|
||
| import com.flipkart.varadhi.entities.Endpoint; | ||
|
|
||
| public record DeliveryResponse(int statusCode, Endpoint.Protocol protocol, byte[] body) { |
There was a problem hiding this comment.
can we keep the Endpoint object itself here?
There was a problem hiding this comment.
not sure if it would be helpful since the caller will already have the endpoint object.
might as well remove the protocol field itself
| .method( | ||
| endpoint.getMethod(), | ||
| ArrayUtils.isEmpty(message.getPayload()) ? HttpRequest.BodyPublishers.noBody() : | ||
| HttpRequest.BodyPublishers.ofByteArray(message.getPayload()) |
There was a problem hiding this comment.
thing to note here:
HttpRequest.BodyPublishers.ofByteArray does not make a copy of the array we are passing, which is good. But turns out that httpClient from jdk, does not support ByteBuffer object directly (and does not support off-heap objects as well.)
If that is what we ned in future, then we will want to look into using netty directly or an http client that does support offheap buffers. I wonder if vertx's http client supports it or not!!
There was a problem hiding this comment.
Interesting. Something to keep in mind. For now jdk's http client is fine.
| .thenApply(response -> new DeliveryResponse(response.statusCode(), endpoint.getProtocol(), | ||
| response.body() | ||
| )) | ||
| .exceptionally(e -> { |
There was a problem hiding this comment.
Is this exceptionally block even required? It does not look like it.
There was a problem hiding this comment.
removed it, had added for testing
| @Override | ||
| public CompletableFuture<DeliveryResponse> deliver(Message message) throws Exception { | ||
| HttpRequest.Builder requestBuilder = HttpRequest.newBuilder() | ||
| .uri(endpoint.getUrl().toURI()) |
There was a problem hiding this comment.
The fact that we are forced to do this endpoint.getUrl().toURI()) , raises a question. Should we also just keep the URI object in the Endpoint object instead of URL?
There was a problem hiding this comment.
sure, I can make this change
No description provided.