[WIP] Fixes #2943 - implements one xDS connection per resource type and config server #4823
[WIP] Fixes #2943 - implements one xDS connection per resource type and config server #4823dmitri-d wants to merge 7 commits intoenvoyproxy:masterfrom dmitri-d:2943-optimize-eds
Conversation
| public: | ||
| virtual ~GrpcMuxFactory() {} | ||
|
|
||
| virtual Config::GrpcMux* |
There was a problem hiding this comment.
Can you add Doxygen comments, thanks.
| std::string mux_key; // config_source_name + type_url | ||
| switch(config_source.grpc_services(0).target_specifier_case()) { | ||
| case envoy::api::v2::core::GrpcService::kEnvoyGrpc: { | ||
| mux_key = config_source.grpc_services(0).envoy_grpc().cluster_name() + type_url; |
There was a problem hiding this comment.
I wonder if we should literally require proto equality here. The reasoning here is that there are configuration options, e.g. timeouts, initial metadata, which should ideally be consistent.
|
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Looks good. Needs tests and release notes, but on the right track.
| * @param local_info LocalInfo::LocalInfo local info. | ||
| * @param async_client Grpc::AsyncClientPtr async client to use for the grpc connection | ||
| * @param dispatcher event dispatcher. | ||
| * @param _method fully qualified name of v2 gRPC API bidi streaming method (as per protobuf |
| virtual ~GrpcMuxFactory() {} | ||
|
|
||
| /** | ||
| * Returns an existing or creates a new GrpcMux. |
There was a problem hiding this comment.
Can you explain the criteria by which we get existing vs. new?
| Stats::Scope& scope, const std::string type_url) override { | ||
|
|
||
| uint64_t proto_hash; // config_source_name + type_url | ||
| switch(config_source.grpc_services(0).target_specifier_case()) { |
There was a problem hiding this comment.
I would just write proto_hash = MessageUtil::hash(config_source.grpc_services(0)
| NOT_REACHED_GCOVR_EXCL_LINE; | ||
| } | ||
|
|
||
| const uint64_t mux_key = proto_hash + XXH64(type_url.c_str(), type_url.length() + 1, 0); |
There was a problem hiding this comment.
Maybe ^ instead of +, or even better, use proto_hash as the seed.
| } | ||
|
|
||
| private: | ||
| std::unordered_map<uint64_t, Config::GrpcMux*> muxes_; |
There was a problem hiding this comment.
GrpcMux should be a unique_ptr here to get cleanup via RAII.
|
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Looks good, just needs tests now.
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
2 participants
@htuch: dmitri-d@aefebec#diff-c0c965f7fd55bbb5ef55f13321e43270R344 is this what you had in mind when you commented on insufficiency of cluster_name/target_uri for map keys?