Protobuf indirection mapping header by htuch · Pull Request #1236 · envoyproxy/envoy

htuch · 2017-07-10T21:30:52Z

Needed for Google import, we have multiple implementations of protobuf internally and need to be able to remap to the appropriate one.

Needed for Google import. This is only part way implemented, looking for early feedback before committing to this approach. Thanks!

htuch · 2017-07-10T21:31:07Z

@mattklein123 @dnoe @lizan @fengli79

mattklein123 · 2017-07-10T21:33:04Z

Fine with me if fine for other G folks.

fengli79

LGTM to the WIP in general.
Can we tests to check:

No #include for protobuf headers out of common/protobuf folder.
No ::google::protobuf out of common/protobuf folder.

fengli79 · 2017-07-11T03:21:51Z

include/envoy/grpc/async_client.h


+#include "common/protobuf/descriptor.h"
+
 #include "google/protobuf/descriptor.h"


remove this?

This and the rest are not done yet as it's WiP - I'll be cleaning up all these refs, thanks.

fengli79 · 2017-07-11T03:31:59Z

source/common/grpc/json_transcoder_filter.h

@@ -62,19 +63,17 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config> {
                   google::protobuf::io::ZeroCopyInputStream& request_input,


Protobuf::Io::ZeroCopyInputStream

fengli79 · 2017-07-11T03:32:28Z

source/common/grpc/json_transcoder_filter.h

-  google::protobuf::util::Status
-  methodToRequestInfo(const google::protobuf::MethodDescriptor* method,
-                      google::grpc::transcoding::RequestInfo* info);
+  google::protobuf::util::Status methodToRequestInfo(const Protobuf::MethodDescriptor* method,


Protobuf::Util::Status

fengli79 · 2017-07-11T03:32:52Z

source/common/grpc/json_transcoder_filter.h

+                                                     google::grpc::transcoding::RequestInfo* info);

 private:
  google::protobuf::DescriptorPool descriptor_pool_;


Protobuf::DescriptorPool

fengli79 · 2017-07-11T03:33:32Z

source/common/grpc/rpc_channel_impl.cc


-void RpcChannelImpl::CallMethod(const ::google::protobuf::MethodDescriptor* method,
+void RpcChannelImpl::CallMethod(const Protobuf::MethodDescriptor* method,
                                ::google::protobuf::RpcController*,


Protobuf::RpcController

fengli79 · 2017-07-11T03:33:53Z

source/common/grpc/rpc_channel_impl.cc

+void RpcChannelImpl::CallMethod(const Protobuf::MethodDescriptor* method,
                                ::google::protobuf::RpcController*,
                                const ::google::protobuf::Message* grpc_request,
                                ::google::protobuf::Message* grpc_response,


fengli79 · 2017-07-11T03:34:07Z

source/common/grpc/rpc_channel_impl.h

  // ::google::protobuf::RpcChannel
-  void CallMethod(const ::google::protobuf::MethodDescriptor* method,
+  void CallMethod(const Protobuf::MethodDescriptor* method,
                  ::google::protobuf::RpcController* controller,


fengli79 · 2017-07-11T03:34:12Z

source/common/grpc/rpc_channel_impl.h

-  void CallMethod(const ::google::protobuf::MethodDescriptor* method,
+  void CallMethod(const Protobuf::MethodDescriptor* method,
                  ::google::protobuf::RpcController* controller,
                  const ::google::protobuf::Message* grpc_request,


fengli79 · 2017-07-11T03:34:19Z

source/common/grpc/rpc_channel_impl.h

+  void CallMethod(const Protobuf::MethodDescriptor* method,
                  ::google::protobuf::RpcController* controller,
                  const ::google::protobuf::Message* grpc_request,
                  ::google::protobuf::Message* grpc_response,


fengli79 · 2017-07-11T03:34:26Z

source/common/grpc/rpc_channel_impl.h

  Http::AsyncClient::Request* http_request_{};
-  const ::google::protobuf::MethodDescriptor* grpc_method_{};
+  const Protobuf::MethodDescriptor* grpc_method_{};
  ::google::protobuf::Message* grpc_response_{};


htuch · 2017-07-11T11:18:40Z

@fengli79 Good idea on automated validation on dependencies. I'll add a check_format step to grep for these.

…ck_format.

htuch · 2017-07-11T15:31:53Z

@mattklein123 @dnoe @lizan @fengli79 This is now ready, PTAL.

mattklein123

looks good, 2 small comments, also needs master merge FYI

mattklein123 · 2017-07-11T16:30:32Z

source/common/protobuf/protobuf.h

@@ -0,0 +1,33 @@
+#pragma once


Can we get some comment/explanation in this file of why we have this?

mattklein123 · 2017-07-11T16:32:04Z

tools/check_format.py

 SUFFIXES = (".cc", ".h", "BUILD")

+# Files in these paths can make reference to protobuf stuff directly
+GOOGLE_PROTOBUF_WHITELIST = ('./ci/prebuilt', './source/common/protobuf')


small ask: This is going to cause issues for us in our import. Would it be possible to do this as a regex somehow where it will match an arbitrary prefix? Or allow the prefix to be passed as a new CLI option?

dnoe · 2017-07-11T17:26:08Z

source/common/protobuf/protobuf.h

+namespace Envoy {
+
+namespace Protobuf = google::protobuf;
+namespace ProtobufWkt = google::protobuf;


What's a Wkt?

dnoe · 2017-07-11T17:30:24Z

source/common/protobuf/protobuf.h

+
+typedef std::string String;
+
+inline const String ToString(const std::string& s) { return s; }


I know the backstory so I understand why this exist but it's pretty weird looking without this context. I'm assuming as written here that it is effectively a no-op with optimizations on.

Yes, this is my assumption, today it's not used for anything on the performance critical path either.

mattklein123 · 2017-07-11T18:31:51Z

tools/check_format.py


+# To avoid breaking the Lyft import, we just check for path inclusion here.
+def whitelistedForProtobufDeps(file_path):
+  return len([prefix in file_path for prefix in GOOGLE_PROTOBUF_WHITELIST]) > 0


nit: s/prefix/path_segment

Right? (Just making sure I'm reading this correctly).

Automatic merge from submit-queue. Remove pessimizing move.

Description: this PR fixes the android release job and caches the release artifacts to prevent double unnecessary compute time. Risk Level: med - confirm working with release tag. Testing: testing done in #1236. However, we will need to cut a release to verify final correctness Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This consolidates the extproc admin server from two separate ports (metricsPort and healthPort) into a single adminPort (default 1064) serving both /metrics and /health endpoints. The `aigw run` and the new `aigw healthcheck` commands now take --admin-port flags, propagating this to extproc. Previously, extproc exposed metrics on port 1064 and health checks on port 1065. Now both are served from a single HTTP server on port 1064, simplifying configuration and Docker HEALTHCHECK setup. The Docker HEALTHCHECK is now functional, using the new healthcheck subcommand which polls the admin server's /health endpoint. This /health endpoint proxies to the extproc gRPC health check, returning HTTP 200 when SERVING. A new subcommand (`aigw healthcheck`) was needed as the image is distroless (has no curl etc). When aigw run starts Envoy, it polls for readiness using either the Envoy admin address (when configured in EnvoyProxy bootstrap) or by checking if the Gateway listener port accepts TCP connections. This ensures startup completes reliably with minimal Gateway configurations. Gateway validation now requires at least one listener to be configured. --------- Signed-off-by: Adrian Cole <adrian@tetrate.io>

WiP: protobuf abstraction library.

36f7b42

Needed for Google import. This is only part way implemented, looking for early feedback before committing to this approach. Thanks!

fengli79 reviewed Jul 11, 2017

View reviewed changes

htuch added 3 commits July 11, 2017 09:25

Further progress, switched to namespace aliasing to simplify.

ce24313

Converted remaining google::protobuf direct references.

6eaab01

Validate that we don't have unexpected direct deps on protobuf in che…

47a70ab

…ck_format.

htuch changed the title ~~WiP: protobuf abstraction library.~~ Protobuf indirection mapping header Jul 11, 2017

mattklein123 reviewed Jul 11, 2017

View reviewed changes

dnoe reviewed Jul 11, 2017

View reviewed changes

htuch added 2 commits July 11, 2017 13:51

Review feedback.

b76a66a

Merge remote-tracking branch 'upstream/master' into pb-macros

a30e9db

mattklein123 reviewed Jul 11, 2017

View reviewed changes

htuch added 2 commits July 11, 2017 14:33

s/prefix/path_segment/ in check_format.

e2ab716

Fix metadata build issue.

761e5cc

mattklein123 approved these changes Jul 11, 2017

View reviewed changes

htuch merged commit a4d7888 into envoyproxy:master Jul 11, 2017

htuch deleted the pb-macros branch July 11, 2017 21:44

taiki45 mentioned this pull request Jan 18, 2018

build: 'check_format.py check' reports line number #2390

Merged

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

Remove pessimizing move. (envoyproxy#1236)

08e2852

Automatic merge from submit-queue. Remove pessimizing move.


		#include "common/protobuf/descriptor.h"

		#include "google/protobuf/descriptor.h"

		@@ -62,19 +63,17 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config> {
		google::protobuf::io::ZeroCopyInputStream& request_input,


		typedef std::string String;

		inline const String ToString(const std::string& s) { return s; }

Conversation

htuch commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Jul 10, 2017

Uh oh!

mattklein123 commented Jul 10, 2017

Uh oh!

fengli79 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!

htuch commented Jul 11, 2017

Uh oh!

htuch commented Jul 11, 2017

Uh oh!

mattklein123 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

htuch commented Jul 10, 2017 •

edited

Loading