Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor resource manager #11182

Merged
merged 5 commits into from
May 15, 2020
Merged

Refactor resource manager #11182

merged 5 commits into from
May 15, 2020

Conversation

tonya11en
Copy link
Member

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

@tonya11en
Refactor resource manager
f1e43cb 
Signed-off-by: Tony Allen <[email protected]>
@tonya11en tonya11en mentioned this pull request May 13, 2020
Merged
@mattklein123 mattklein123 self-assigned this May 13, 2020
@tonya11en
Copy link
Member Author

/wait

@tonya11en
stats integration test
fb11463 
Signed-off-by: Tony Allen <[email protected]>
@lizan lizan requested a review from Shikugawa May 13, 2020 23:56
Shikugawa
Shikugawa previously approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@Shikugawa Shikugawa left a comment

Choose a reason for hiding this comment

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

LGTM. Nice cleanup. Thank you!

@ggreenway
Copy link
Contributor

merge conflict needs fixing

/wait

ggreenway
ggreenway previously approved these changes May 14, 2020
View reviewed changes
@ggreenway
Copy link
Contributor

stats test still failing

/wait

@tonya11en
stats
7e0775b 
Signed-off-by: Tony Allen <[email protected]>
mattklein123
mattklein123 reviewed May 14, 2020
View reviewed changes
test/integration/stats_integration_test.cc
@@ -286,7 +287,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
// If you encounter a failure here, please see
// 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);
Copy link
Member

Choose a reason for hiding this comment

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

qq: Why does the memory change here? I would expect no change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed it was because each cluster has its own resource manager and I'm fiddling with the resource type?

Copy link
Member

Choose a reason for hiding this comment

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

But what changes are you making that cause it to grow in size? Is it padding or something? cc @jmarantz

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

OK that's probably fine, it just seemed like a pretty large bump for no functionality change.

@mattklein123
Copy link
Member

Can you merge master, I think it will fix coverage. Thank you!

/wait

@ggreenway ggreenway merged commit 2453630 into envoyproxy:master May 15, 2020
antoniovicente
antoniovicente reviewed May 19, 2020
View reviewed changes
source/common/upstream/resource_manager_impl.h
remaining_(remaining) {
remaining_.set(max);
}
~ResourceImpl() override { ASSERT(current_ == 0); }

// Upstream::Resource
bool canCreate() override { return current_ < max(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Isn't this equivalent to the base class implementation?

source/common/upstream/resource_manager_impl.h
updateRemaining();
open_gauge_.set(canCreate() ? 0 : 1);
open_gauge_.set(BasicResourceLimitImpl::canCreate() ? 0 : 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why BasicResourceLimitImpl::canCreate() instead of just canCreate()?

test/integration/stats_integration_test.cc
@@ -333,6 +334,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) {
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: upper limit remains at 36800

PiotrSikora pushed a commit that referenced this pull request Jun 30, 2020
@lizan
Refactor resource manager (#11182)
9258bfe 
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 <[email protected]>
Signed-off-by: Lizan Zhou <[email protected]>
PiotrSikora pushed a commit that referenced this pull request Jun 30, 2020
@lizan
Refactor resource manager (#11182)
0f26e68 
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 <[email protected]>
Signed-off-by: Lizan Zhou <[email protected]>
PiotrSikora pushed a commit that referenced this pull request Jun 30, 2020
@tonya11en @PiotrSikora
Refactor resource manager (#11182)
c23fa42 
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 <[email protected]>
brian-avery pushed a commit to istio/envoy that referenced this pull request Jun 30, 2020
@tonya11en
Refactor resource manager (envoyproxy#11182)
9bfd872 
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 <[email protected]>
fpesce pushed a commit to istio/envoy that referenced this pull request Jun 30, 2020
@jplevyak @lizan
Refactor resource manager (envoyproxy#11182) (#36)
acf2f39 
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 <[email protected]>
Signed-off-by: Lizan Zhou <[email protected]>

Co-authored-by: Lizan Zhou <[email protected]>
brian-avery pushed a commit to istio/envoy that referenced this pull request Jun 30, 2020
@jplevyak @lizan
Refactor resource manager (envoyproxy#11182) (#35)
fc7bc67 
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 <[email protected]>
Signed-off-by: Lizan Zhou <[email protected]>

Co-authored-by: Lizan Zhou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@antoniovicente antoniovicente antoniovicente left review comments

@ggreenway ggreenway ggreenway approved these changes

@Shikugawa Shikugawa Shikugawa left review comments

Assignees

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tonya11en @ggreenway @mattklein123 @Shikugawa @antoniovicente