Refactor resource manager#11182
Conversation
Signed-off-by: Tony Allen <tony@allen.gg>
|
/wait |
Signed-off-by: Tony Allen <tony@allen.gg>
Shikugawa
left a comment
There was a problem hiding this comment.
LGTM. Nice cleanup. Thank you!
|
merge conflict needs fixing /wait |
Signed-off-by: Tony Allen <tony@allen.gg>
|
stats test still failing /wait |
| // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests | ||
| // for details on how to fix. | ||
| EXPECT_MEMORY_EQ(m_per_cluster, 44233); | ||
| EXPECT_MEMORY_EQ(m_per_cluster, 44425); |
There was a problem hiding this comment.
qq: Why does the memory change here? I would expect no change?
There was a problem hiding this comment.
I assumed it was because each cluster has its own resource manager and I'm fiddling with the resource type?
There was a problem hiding this comment.
But what changes are you making that cause it to grow in size? Is it padding or something? cc @jmarantz
There was a problem hiding this comment.
I think it's because ManagedResourceImpl has a concrete base class. All of that functionality used to be in the same class, and there's probably a difference in padding due to different ordering of the members between the two classes, or something like that.
There was a problem hiding this comment.
OK that's probably fine, it just seemed like a pretty large bump for no functionality change.
|
Can you merge master, I think it will fix coverage. Thank you! /wait |
Signed-off-by: Tony Allen <tony@allen.gg>
| ~ResourceImpl() override { ASSERT(current_ == 0); } | ||
|
|
||
| // Upstream::Resource | ||
| bool canCreate() override { return current_ < max(); } |
There was a problem hiding this comment.
nit: Isn't this equivalent to the base class implementation?
| BasicResourceLimitImpl::inc(); | ||
| updateRemaining(); | ||
| open_gauge_.set(canCreate() ? 0 : 1); | ||
| open_gauge_.set(BasicResourceLimitImpl::canCreate() ? 0 : 1); |
There was a problem hiding this comment.
nit: why BasicResourceLimitImpl::canCreate() instead of just canCreate()?
| // 2020/04/07 10661 35557 36000 fix clang tidy on master | ||
| // 2020/04/23 10531 36281 36800 http: max stream duration upstream support. | ||
| // 2020/05/05 10908 36345 36800 router: add InternalRedirectPolicy and predicate | ||
| // 2020/05/13 10531 36537 44600 Refactor resource manager |
There was a problem hiding this comment.
nit: upper limit remains at 36800
This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base. Signed-off-by: Tony Allen <tony@allen.gg> Signed-off-by: Lizan Zhou <lizan@tetrate.io>
This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base. Signed-off-by: Tony Allen <tony@allen.gg> Signed-off-by: Lizan Zhou <lizan@tetrate.io>
This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base. Signed-off-by: Tony Allen <tony@allen.gg>
This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base. Signed-off-by: Tony Allen <tony@allen.gg>
This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base. Signed-off-by: Tony Allen <tony@allen.gg> Signed-off-by: Lizan Zhou <lizan@tetrate.io> Co-authored-by: Lizan Zhou <lizan@tetrate.io>
This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base. Signed-off-by: Tony Allen <tony@allen.gg> Signed-off-by: Lizan Zhou <lizan@tetrate.io> Co-authored-by: Lizan Zhou <lizan@tetrate.io>
This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base.
Risk Level: Low, no changes to functionality
Testing: New UT
Docs Changes: n/a
Release Notes: n/a
Relates to #11028