Fix ABI: all data is pulled by the code in the VM.#274
Fix ABI: all data is pulled by the code in the VM.#274jplevyak merged 11 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
PiotrSikora
left a comment
There was a problem hiding this comment.
It seems that this change passes only number of body bytes in the function calls, but not any other values that would be similarly useful, IMHO (i.e. number of headers, trailers, status message length, status code). Is there any particular reason for this choice?
api/wasm/cpp/proxy_wasm_api.h
Outdated
| virtual void onHttpCallResponse(uint32_t token, std::unique_ptr<WasmData> header_pairs, | ||
| std::unique_ptr<WasmData> body, | ||
| std::unique_ptr<WasmData> trailer_pairs); | ||
| virtual void onHttpCallResponse(uint32_t token, size_t body_size); |
There was a problem hiding this comment.
This seems like a weird choice, why are we only passing body size, but not number of headers / trailers?
There was a problem hiding this comment.
Because the call to get the body requires the length requested, so it would require an extra call to get the length of the buffer before getting the whole body.
| return std::make_unique<WasmData>(value_ptr, value_size); | ||
| } | ||
|
|
||
| inline std::pair<uint32_t, WasmDataPtr> getStatus() { |
There was a problem hiding this comment.
Could this at least indicate what the status this is about? We're ending up with a lot of functions that are way too generic, IMHO.
This seems to be used only in onGrpcClose and onHttpCallResponse. Could this be named getCalloutStatus or something like that?
There was a problem hiding this comment.
And for generic WasmResult != Ok. It would be nice to have an addition details message available for debugging.
I see this more googl3 Status which is completely generic. It has error namespaces, perhaps you would be interested in adding that? e.g. GrpcErrors, HttpErrors, WasmErrors ?
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
|
Updated WRT comments. PTAL |
| // A unique name for a filters/services in a VM for use in identifiying the filter/service if | ||
| // multiple filters/services are handled by the same vm_id and root_id and for logging/debugging. | ||
| // If left blank the plugin will be assigned an integer name corresponding to when it registers | ||
| // with the VM (i.e. the first plugin to register with the VM will be "0"). |
There was a problem hiding this comment.
Why? Is it only to identify them? UUID might be better, then, and it's already used for that purpose in Envoy.
There was a problem hiding this comment.
Removed it. This is purely user-facing so if they don't add one then they done need it.
| std::unique_ptr<WasmData> trailer_pairs)>; | ||
| using GrpcSimpleCallCallback = | ||
| std::function<void(GrpcStatus status, std::unique_ptr<WasmData> message)>; | ||
| std::function<void(uint32_t, size_t, uint32_t)>; // headers, body_size, trailers |
There was a problem hiding this comment.
headers / trailers - is that a number of items or total length when serialized?
| virtual void onFailure(GrpcStatus status, std::unique_ptr<WasmData> error_message) = 0; | ||
| virtual void onCreateInitialMetadata(uint32_t /* headers */) {} | ||
| virtual void onSuccess(size_t body_size) = 0; | ||
| virtual void onFailure(GrpcStatus status) = 0; |
There was a problem hiding this comment.
Should this include status_message_size?
There was a problem hiding this comment.
We could. My feeling however is that this is an exceptional situation and if you have an error and need to the details, then an extra call will not kill you.
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak jplevyak@gmail.com