thrift_proxy: introduce MessageMetadata to track message headers and other metadata by zuercher · Pull Request #3991 · envoyproxy/envoy

zuercher · 2018-07-30T22:44:04Z

In advance of implementing the header transport and "ttwitter" protocol, refactor
the message metadata (method name, message type, sequence id) into a metadata object.
The MessageMetadata also tracks the protocol (provided by the header transport) and
app exception data for exceptions triggered by the transport before the message begins.

Risk Level: low
Testing: existing tests
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Stephan Zuercher stephan@turbinelabs.io

…other metadata In advance of implementing the header transport and "ttwitter" protocol, refactor the message metadata (method name, message type, sequence id) into a metadata object. The MessageMetadata also tracks the protocol (provided by the header transport) and app exception data for exceptions triggered by the transport before the message begins. *Risk Level*: low *Testing*: existing tests *Docs Changes*: n/a *Release Notes*: n/a Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

brian-pane · 2018-07-31T18:23:33Z

source/extensions/filters/network/thrift_proxy/binary_protocol_impl.cc


  if (name_len > 0) {
-    name.assign(std::string(static_cast<char*>(buffer.linearize(name_len)), name_len));
+    metadata.setMethodName(std::string(static_cast<char*>(buffer.linearize(name_len)), name_len));


[minor] The cast ideally should be to const char* rather than char*.

should there be a check for name_len's max value? Otherwise an ill formed message could trigger big allocations... Or is buffer.length() checked for max length somewhere else? Even so, should we check for name_len's max here?

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher · 2018-08-01T16:29:21Z

@brian-pane ptal

htuch

Looks good. A few questions/nits; general plea for small PRs (and for major cleanups, to make them as mechanical as possible). I know that's not always pragmatic, but where plausible.

htuch · 2018-08-01T23:04:14Z

source/extensions/filters/network/thrift_proxy/app_exception_impl.cc

-  proto.writeMessageBegin(buffer, method_name_, ThriftProxy::MessageType::Exception, seq_id_);
+void AppException::encode(MessageMetadata& metadata, ThriftProxy::Protocol& proto,
+                          Buffer::Instance& buffer) const {
+  ASSERT(metadata.hasMethodName());


Can you comment on how this is known to be true?

Roughly because of the locations where the class is instantiated, but I'll need it to handle their absence, so I'll just add that.

htuch · 2018-08-01T23:07:01Z

source/extensions/filters/network/thrift_proxy/conn_manager.cc

+    return;
+  } catch (const AppException& ex) {
+    ENVOY_LOG(error, "thrift application exception: {}", ex.what());
+    if (!rpcs_.empty()) {


Nit: Prefer to invert the logic here.

htuch · 2018-08-01T23:16:06Z

source/extensions/filters/network/thrift_proxy/decoder.cc

    }
    ENVOY_LOG(debug, "thrift: {} transport started", transport_->name());

+    if (metadata_->hasProtocol()) {


This looks like a behavioral change; ideally we would split that out from the refactor around metadata that is happening.

htuch · 2018-08-01T23:17:33Z

source/extensions/filters/network/thrift_proxy/metadata.cc

+  }
+
+  for (auto i = headers_.begin(), j = rhs.headers_.begin(); i != headers_.end(); ++i, ++j) {
+    if (i->key() != j->key() || i->value() != j->value()) {


Is the map inherently ordered?

At this point it is. I'll discuss more in the question about HeaderMap below.

htuch · 2018-08-01T23:18:16Z

source/extensions/filters/network/thrift_proxy/metadata.h

+/*
+ * HeaderMap contains Thrift transport and/or protocol-level headers.
+ */
+class HeaderMap {


Why do we need a new HeaderMap abstraction? Could we reuse the existing HTTP one via an adapter or just use a limited surface of its APIs?

I considered using the Http::HeaderMap. I like the idea of reusing LowerCaseString and HeaderEntryImpl and it's memory management. I balked because it seems likely that most of the 70-odd inline headers will never see use in Thrift. (The non-inline headers are stored in a list, as in the local implementation). I suppose using the Http::HeaderMap will make my life easier when I add logging, since that API takes them. I'll have another look at reusing it.

FWIW, at some point I would like to make the inline headers that the HeaderMapImpl supports runtime configurable. I think this is possible with some work. Obviously not suggesting you do that, but I thought I would throw that out there. I think that's potentially interesting when private filters want to add extra inline headers and/or we would like to prune some of the inline headers that might not be used in all installations.

That would be awesome.

@htuch do you mind if I leave this as-is for now and reconsider it when we're actually able to use headers for routing?

Yeah, but if you could either put in a TODO or file an issue to track this it'd be great, whatever you think is appropriate.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

rgs1

I see test/extensions/filters/network/thrift_proxy/tmp as part of the first commit, did that get in by accident?

htuch

Can you take a look at the merge conflict? I think this looks good to go otherwise based on surface analysis.

htuch · 2018-08-02T17:05:41Z

source/extensions/filters/network/thrift_proxy/metadata.h

+/*
+ * HeaderMap contains Thrift transport and/or protocol-level headers.
+ */
+class HeaderMap {


Yeah, but if you could either put in a TODO or file an issue to track this it'd be great, whatever you think is appropriate.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher · 2018-08-06T16:33:22Z

Added a todo, removed the stray file, and merged master.

zuercher requested a review from brian-pane July 30, 2018 22:44

zuercher added 2 commits July 30, 2018 16:03

add comments/tests for MessageMetadata

e105fd7

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

improve code coverage

0f45fa5

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

mattklein123 assigned brian-pane Jul 31, 2018

brian-pane previously approved these changes Jul 31, 2018

View reviewed changes

cast linearized buffers to const char*

5f6d95a

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher dismissed brian-pane’s stale review via 5f6d95a August 1, 2018 16:24

brian-pane previously approved these changes Aug 1, 2018

View reviewed changes

zuercher mentioned this pull request Aug 1, 2018

thrift_proxy: fix oneway bugs #4025

Merged

htuch self-assigned this Aug 1, 2018

htuch reviewed Aug 1, 2018

View reviewed changes

zuercher added 2 commits August 1, 2018 16:24

Merge branch 'master' into stephan/thrift-message-metadata

e8bf225

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

review comments (except HeaderMap)

bdd032f

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher dismissed brian-pane’s stale review via bdd032f August 2, 2018 01:57

rgs1 reviewed Aug 3, 2018

View reviewed changes

htuch previously approved these changes Aug 3, 2018

View reviewed changes

zuercher added 2 commits August 6, 2018 09:14

add todo, remove stray file

fd1ed20

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

Merge branch 'master' into stephan/thrift-message-metadata

ccff5c8

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>

zuercher dismissed htuch’s stale review via ccff5c8 August 6, 2018 16:17

htuch approved these changes Aug 7, 2018

View reviewed changes

htuch merged commit b14dee5 into envoyproxy:master Aug 7, 2018

Conversation

zuercher commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher commented Aug 1, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher commented Aug 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rgs1 Aug 2, 2018 •

edited

Loading