thrift: implement Twitter protocol variant #4363
Conversation
Implements the Twitter variant of the Thrift binary protocol, as implemented by the finagle library. *Risk Level*: low *Testing*: unit tests *Docs Changes*: updated API docs *Release Notes*: n/a Signed-off-by: Stephan Zuercher <[email protected]>
|
@brirams, @rgs1, @fishcakez if you'd like to have a look |
| ClientId(const ThriftStructValue& value) { | ||
| for (const auto& field : value.fields()) { | ||
| if (field->fieldId() == NameFieldId) { | ||
| name_ = field->getValue().getValueTyped<std::string>(); |
There was a problem hiding this comment.
should we log when we see more than one client id (e.g.: name_ gets set more than once)? also, should we document why we keep the last occurence?
There was a problem hiding this comment.
There isn't any provision for sending multiple client ids. The loop here is over the fields of the ClientId struct (which has only ever had a single field, with default requiredness). The struct itself is optional as well.
Signed-off-by: Stephan Zuercher <[email protected]>
Signed-off-by: Stephan Zuercher <[email protected]>
|
@htuch do you have time to take a quick pass at this in the next couple of days? |
Signed-off-by: Stephan Zuercher <[email protected]>
Signed-off-by: Stephan Zuercher <[email protected]>
Signed-off-by: Stephan Zuercher <[email protected]>
|
@zuercher if you can hold off until Mon, I'll do this then. |
|
Sure. Will fix the merge conflict in the meantime. |
Signed-off-by: Stephan Zuercher <[email protected]>
Signed-off-by: Stephan Zuercher <[email protected]>
There was a problem hiding this comment.
I'm going to defer completely to @rgs1 and @brian-pane on the correctness here, a quick pass revealed a few nits. Feel free to ship when they are resolved as you think best.
| @@ -91,14 +91,27 @@ void ConnectionManager::dispatch() { | |||
| void ConnectionManager::sendLocalReply(MessageMetadata& metadata, const DirectResponse& response) { | |||
| Buffer::OwnedImpl buffer; | |||
|
|
|||
| response.encode(metadata, *protocol_, buffer); | |||
| DirectResponse::ResponseType result = response.encode(metadata, *protocol_, buffer); | |||
| @@ -41,7 +45,19 @@ bool AutoProtocolImpl::readMessageBegin(Buffer::Instance& buffer, MessageMetadat | |||
|
|
|||
| uint16_t version = buffer.peekBEInt<uint16_t>(); | |||
| if (BinaryProtocolImpl::isMagic(version)) { | |||
| setType(ProtocolType::Binary); | |||
| // 12 bytes is the minimum length for message-begin in the binary protocol. | |||
| if (buffer.length() < 12) { | |||
There was a problem hiding this comment.
Nit: raw 12 here feels icky, unclear how it relates to following code and its expectations. It might not be much clearer to add a constant at top of file, but for your consideration.
| const SpanList& spans() const { return spans_; } | ||
|
|
||
| /** | ||
| * @return SpanList* a pointer to a mutable list of Spans |
There was a problem hiding this comment.
Nit: prefer Envoy style SpanList& unless this can be null.
| F(endpointStruct, Endpoint) \ | ||
| F(upgradeReplyStruct, UpgradeReply) | ||
|
|
||
| #define REQUEST_HEADER_FIELD_NAMES(F) \ |
There was a problem hiding this comment.
Why not make all of these a const singleton?
Signed-off-by: Stephan Zuercher <[email protected]>
Signed-off-by: Stephan Zuercher <[email protected]>
|
@htuch I've addressed the review comments, but I still need a maintainer to actually click the button. |
Implements the Twitter variant of the Thrift binary protocol,
as implemented by the finagle library.
Risk Level: low
Testing: unit tests
Docs Changes: updated API docs
Release Notes: n/a
Signed-off-by: Stephan Zuercher [email protected]