Skip to content

async client refactor#92

Merged
mattklein123 merged 3 commits intomasterfrom
async_client_router
Sep 22, 2016
Merged

async client refactor#92
mattklein123 merged 3 commits intomasterfrom
async_client_router

Conversation

@mattklein123
Copy link
Copy Markdown
Member

We have increasing use cases where we want Envoy filters to be able to make
direct HTTP calls but use the full router feature set (retries, etc.). We also want
all of the same stats. Previously, the HTTP async client didn't support most of
the router features and also had some code duplication. This change makes the
HTTP async client use the router directly so that all standard features are
available via request headers directly.

In follow ups we will add the ability for the async client to write to an access log,
as well as for a constant route to be passed in, which will be higher
performance than having to use request headers that need to be parsed.

@mattklein123
Copy link
Copy Markdown
Member Author

@lyft/network-team

@@ -124,9 +124,6 @@ void RpcChannelImpl::onFailure(Http::AsyncClient::FailureReason reason) {
case Http::AsyncClient::FailureReason::Reset:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that this is the only possibility, maybe the parameter can be removed completely? Does FailureReason even need to exist now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth on this. Since the enum is already there and plumbed everywhere, it would be a lot of work to change it to no parameters, then we might need a parameter again. I'm inclined to leave it for now?

void healthCheck(bool is_hc) override { hc_request_ = is_hc; }

const std::string& protocol_;
SystemTime start_time_;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

: protocol_(protocol), start_time_(std::chrono::system_clock::now()) {}

// Http::AccessLog::RequestInfo
SystemTime startTime() const override { return start_time_; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const SystemTime& ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an integer, no point in returning a reference

void LightStepSink::onSuccess(Http::MessagePtr&&) { stats_.collector_success_.inc(); }
void LightStepSink::onSuccess(Http::MessagePtr&& response) {
uint64_t response_code = Http::Utility::getResponseStatus(response->headers());
if (response_code == enumToInt(Http::Code::OK)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor, might be better to CodeUtility::is2xx(response_code)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@wgallagher
Copy link
Copy Markdown

👍

@mattklein123 mattklein123 merged commit d3db6ea into master Sep 22, 2016
@mattklein123 mattklein123 deleted the async_client_router branch September 22, 2016 21:37
lspas-volvo pushed a commit to sel-vcc/envoy that referenced this pull request Sep 18, 2019
network: fix misaligned memory access in setsockopt().
arminabf pushed a commit to arminabf/envoy that referenced this pull request Jun 5, 2024
mathetake added a commit that referenced this pull request Mar 3, 2026
Per #90, we will migrate to dynamic modules in the second or third
version of
AI Gateway, the public configuration package must not be coupled with
extproc (see the description in the issue). This renames `extprocconfig`
to more generic (and accurate) name as `filterconfig`.

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
mathetake added a commit that referenced this pull request Mar 3, 2026
**Commit Message**:

Previously, filterconfig package was named extprocconfig
and renamed to it in #92. This fixes a few places still 
mentioning the old name.

**Related Issues/PRs (if applicable)**:

follow up on #92

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants