tech debt: move connection and async client APIs to thread local cluster#14332
tech debt: move connection and async client APIs to thread local cluster#14332mattklein123 merged 11 commits intomasterfrom
Conversation
This is part of several follow ups to #13906. This change moves various functions from the cluster API to the thread local cluster API. This simplifies error handling and also will increase performance slightly as many instances of double map lookups are now a single lookup. Signed-off-by: Matt Klein <mklein@lyft.com>
|
@alyssawilk assigning you since you have been pretty close to this code. I didn't realize how awful and large this was going to be when I started, but it's the right thing to do and will make a final follow up change that I have simpler. I don't think there is any great way to break this apart but the actual prod changes should be easy to review. |
Signed-off-by: Matt Klein <mklein@lyft.com>
alyssawilk
left a comment
There was a problem hiding this comment.
Awesome clean-up! The one change I was having trouble following was why this removes the necessity of the various config validation bits. Just to confirm, they were always shim classes required by prior API organization but not actually validating any config?
They were shims previously, but now I'm actually questioning whether I did this correctly, because it's possible that the way I did it will actually make real networking calls when validating config which seems wrong. Let me look at this again and make sure I did this correctly and also fix your other comments. |
…r_refactor Signed-off-by: Matt Klein <mklein@lyft.com>
…r_refactor Signed-off-by: Matt Klein <mklein@lyft.com>
|
@alyssawilk updated. Thanks for poking me on the validation code. It's actually fairly confusing and not well defined/tested what the validation code is supposed to do but I think the way I have it now is more appropriate. I also fixed more cases that should have had error handling but did not. |
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM modulo that one squash issue (since I'm out tomorrow)
If you don't need that check, feel free to submit. If you do, hopefully someone else can stamp that one liner :-)
|
|
||
| in_flight_request_ = | ||
| cm_.httpAsyncClientForCluster(config_->clusterName()) | ||
| cm_.getThreadLocalCluster(config_->clusterName()) |
There was a problem hiding this comment.
do we need another thread local cluster check here?
There was a problem hiding this comment.
Technically I think yes, but I couldn't quickly sort out how to test this and this code is not clearly used/owned so I will admit I punted. Also, it's guarded behind a previous check, so it would be a rare condition to hit this becoming null but it could happen. Let me add a TODO for this to follow up later since it's a pre-existing bug.
| // A thread local cluster is never guaranteed to exist (because it is not created until warmed) | ||
| // so all calling code must have error handling for this case. Returning nullptr here prevents | ||
| // any calling code creating real outbound networking during validation. | ||
| return nullptr; |
There was a problem hiding this comment.
Awesome, this makes more sense to me now, thanks
…r_refactor Signed-off-by: Matt Klein <mklein@lyft.com>
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM as long as you'll take a look in the not too distant future. May be a preexisting bug but I'd rather have an untested fix than a known bug, and I'd rather have a tested fix than either :-)
Yeah I will do it. I would like to land this to avoid merge conflicts that keep happening. |
This is part of several follow ups to #13906. This change moves various
functions from the cluster API to the thread local cluster API. This
simplifies error handling and also will increase performance slightly
as many instances of double map lookups are now a single lookup.
Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A