WIP: ingestion API protocol#5024
Conversation
|
We are not committing to gRPC. This is only the protocol definition. Right now I am investigating ConnectRPC which seems promising so far. |
| // optional fields | ||
| optional string scope = 2; | ||
| optional string limit_key = 3; | ||
| optional string service = 4; |
There was a problem hiding this comment.
service key as well?
There was a problem hiding this comment.
I think that's invocation specific
There was a problem hiding this comment.
it doesn't hurt to have it here imo, and it's free anyway. i can see a use case where you wanna send records to always the same key
| optional string limit_key = 3; | ||
| optional string service = 4; | ||
| optional string handler = 5; | ||
| map<string, string> headers = 6; |
There was a problem hiding this comment.
do these sum with the Overrides? or Overrides replace them all?
I think it makes sense they sum.
There was a problem hiding this comment.
it merges with the defaults. So it doesn't replace them all. Only the set values
| optional string handler = 4; | ||
| // These headers are merged with the default headers | ||
| // from the Settings message. | ||
| map<string, string> headers = 5; |
There was a problem hiding this comment.
then maybe give it a name like additional_headers -> indicating it adds to the headers in settings.
| // Required when the target service is a virtual object (VO). | ||
| optional string key = 2; | ||
| optional string tracing_parent = 3; | ||
| optional Overrides overrides = 4; |
There was a problem hiding this comment.
can we inline these? it's more efficient.
There was a problem hiding this comment.
Sure thing, it just felt cleaner to group them. No strong opinion
| uint64 offset = 1; | ||
| // Required when the target service is a virtual object (VO). | ||
| optional string key = 2; | ||
| optional string tracing_parent = 3; |
There was a problem hiding this comment.
w3c-tracecontext format right? in that case, just give it that name.
| ErrorKind_UNKNOWN = 0; | ||
| // Server is shutting down and | ||
| // can't accept more records | ||
| SHUTTING_DOWN = 1; |
There was a problem hiding this comment.
unclear what am i supposed to do in this case.
| message Error { | ||
| ErrorKind kind = 1; | ||
| string message = 2; | ||
| } |
There was a problem hiding this comment.
for the errors related to schema registry and co, how about we return an InvocationError in same format we use in ingress? (404 for not found, idr what error code for provided limit key without scope, etc etc).
There was a problem hiding this comment.
This way error handling can be unified among regular ingress usage and the ingress ingestion api
Summary: A seed for the ingestion protocol protobuf.
|
|
||
| syntax = "proto3"; | ||
|
|
||
| package restate.ingestion; |
There was a problem hiding this comment.
| package restate.ingestion; | |
| package dev.restate.ingress.ingestion; |
| // optional fields | ||
| optional string scope = 2; | ||
| optional string limit_key = 3; | ||
| optional string service = 4; |
There was a problem hiding this comment.
it doesn't hurt to have it here imo, and it's free anyway. i can see a use case where you wanna send records to always the same key
| optional string traceparent = 3; | ||
| optional string tracestate = 4; |
There was a problem hiding this comment.
are these in format w3c tracecontext? if yes, can you please add it as comment?
| message Record { | ||
| uint64 offset = 1; | ||
| // Required when the target service is a virtual object (VO). | ||
| optional string key = 2; |
| // from the Settings message. | ||
| map<string, string> additional_headers = 9; | ||
|
|
||
| bytes payload = 100; |
There was a problem hiding this comment.
up to 15 included you use one byte for the field varint, for a field that we're gonna always set use field numbers 1 -> 15 pls
| // (think Kafka cluster + topic + partition). | ||
| // An explicitly empty producer ID is valid but disables | ||
| // deduplication for the entire stream. | ||
| string producer_id = 1; |
There was a problem hiding this comment.
As the protocol stands, i can in theory change this value during the stream, sending another settings with a different producer_id, is it a good idea to allow this?
| message Error { | ||
| ErrorKind kind = 1; | ||
| string message = 2; | ||
| } |
There was a problem hiding this comment.
A related question here, what is the expectation for a client when receiving this Error message? that the server will afterwards tear down the stream?
2 participants
Summary:
A seed for the ingestion protocol protobuf.
Stack created with Sapling. Best reviewed with ReviewStack.