grid: the basic structure of the connectivity grid#15656
grid: the basic structure of the connectivity grid#15656alyssawilk merged 12 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Quick driveby comment: I'm not a fan of "grid" for the name. Without reading the linked issue, I had no idea what that meant. I'd prefer something like "probe" or "autodetect" or something. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
left a comment
There was a problem hiding this comment.
Thanks for the quick review!
I'm still trying to sort out that test error - doesn't reproduce locally with -c opt, but I'm 95% sure it's just a bug in the real grid test, so I think fine for another code pass.
| std::list<ConnectionPool::InstancePtr> pools_; | ||
| // True iff under the stack of the destructor, to avoid calling drain | ||
| // callbacks on deletion. | ||
| bool destroying_{}; |
There was a problem hiding this comment.
I prefer that syntax but I tend to get nits from other reviewers to use {} so I think it's general Envoy style.
|
Quick driveby comment: I'm not a fan of "grid" for the name. Without reading the linked issue, I had no idea what that meant. I'd prefer something like "probe" or "autodetect" or something. probe_pool or autodetect pool don't really work for me. David, any thoughts on naming? |
| PoolIterator pool_it_; | ||
| // The handle to cancel the request to the current pool. | ||
| // This is owned by the pool which created it. | ||
| Cancellable* cancellable_; |
There was a problem hiding this comment.
Since this can't be null, should it be a reference instead (apologies if this is wrong, it's a part of the Envoy style guide that I'm not used to)
There was a problem hiding this comment.
Yeah, Envoy is generally pretty anti-pointer, but this is set by one of the rare Envoy functions which returns a pointer, and we change what it points to, so I'm inclined to leave it as is. swapping the thing a reference refers to just feels wrong. happy to be overriden by Matt here.
There was a problem hiding this comment.
Thanks for the explanation, happy to defer to the two of you here :)
There was a problem hiding this comment.
Optimally this would change to an RAII thing, but this is fine!
|
We could always call it the EyeballHappinessGenerator but that's somewhat of a mouthful :P |
DavidSchinazi
left a comment
There was a problem hiding this comment.
Thanks for the discussion!
source/common/http/conn_pool_grid.cc
Outdated
| // If this point is reached, all pools have been tried. Pass the pool failure up to the | ||
| // original caller. | ||
| ConnectionPool::Callbacks& callbacks = inner_callbacks_; | ||
| grid_.wrapped_callbacks_.erase(index_); // Functionally "delete this" |
There was a problem hiding this comment.
nit: dot after comment (same below)
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
DavidSchinazi
left a comment
There was a problem hiding this comment.
I like the list approach!
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
First pass reviewed, and I've merged past the mega quic merge, @mattklein123 you up for a pass? |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this LGTM at a high level to ship and iterate.
| PoolIterator pool_it_; | ||
| // The handle to cancel the request to the current pool. | ||
| // This is owned by the pool which created it. | ||
| Cancellable* cancellable_; |
There was a problem hiding this comment.
Optimally this would change to an RAII thing, but this is fine!
| // TODO(#15649) track pools with successful connections: don't always start at | ||
| // the front of the list. | ||
| auto wrapped_callback = | ||
| std::make_unique<WrapperCallbacks>(*this, decoder, pools_.begin(), callbacks); |
There was a problem hiding this comment.
This is where we would race, right? Just want to check my understanding.
There was a problem hiding this comment.
Mostly. Right now wrapped callbacks only allows one connection attempt at a time. I've got a follow-up which splits some of the state into PerConnectAttempt and kicks off parallel attempts based on a timer.
Naive connectivity grid, which works for TCP/UDP and only does serial connections.
I figured there's enough code to start with, that all the fun stuff can come in follow-ups.
Risk Level: n/a (unused code)
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a
Part of #15649