Support headers

[mxpv]

This PR adds headers support to ttrpc.

Signed-off-by: Maksym Pavlenko makpav@amazon.com

[crosbymichael]

LGTM

[crosbymichael]

I tested this and have code to update the namespaces package in containerd to pass the context's namespace to ttrpc services

[cpuguy83]

Just a nit, does it make sense to call this Headers when it's not really a header? Seems like metadata to attach to a request.

re: using this for namespaces. Why wouldn't the namespace be passed as an argument to the rpc request?

[mxpv]

Just a nit, does it make sense to call this Headers when it's not really a header? Seems like metadata to attach to a request.

This can be called in many different ways, but the concept remains the same - send arbitrary data with a request. IMO name 'headers' make the intention of this a bit clearer for a user - pass an extra information with a request, similarly to what http headers are used for.

Why wouldn't the namespace be passed as an argument to the rpc request?

That would work, but you'd need to modify each request to add 'namespace' field to use namespaces. In containerd there is already a good way to do this: client, err := containerd.New(address, containerd.WithDefaultNamespace("docker")) <-- all requests from this client use docker ns or

docker = namespaces.WithNamespace(context, "docker")
containerd, err := client.NewContainer(docker, "id")

So it'd be nice to use the same mechanism for ttrpc as well.

[cpuguy83]

Headers are items that are sent at the beginning of a request. These really aren't headers. Metadata would be fitting, IMO. Also similar to grpc.

I'm not sure I agree with stuffing data into the protocol that changes the behavior of the rpc endpoint.. but I guess that's not really an argument for this repo.

Being able to attach metadata to a request does make sense though, e.g. for distributed tracing.

[crosbymichael]

Changing the name from headers to metadata sounds like a good idea

Michael Crosby

On May 24, 2019, at 6:56 PM, Brian Goff notifications@github.com wrote:

Headers are items that are sent at the beginning of a request. These really aren't headers. Metadata would be fitting, IMO. Also similar to grpc.

I'm not sure I agree with stuffing data into the protocol that changes the behavior of the rpc endpoint.. but I guess that's not really an argument for this repo.

Being able to attach metadata to a request does make sense though, e.g. for distributed tracing.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[mxpv]

@crosbymichael @cpuguy83 done

[crosbymichael]

LGTM

[stevvooe]

43 covers the concerns here.

[mxpv]

43 covers the concerns here.

@stevvooe a bit late for the party :( Let me know if anything left to address.