-
Notifications
You must be signed in to change notification settings - Fork 105
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.
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
Introduce manager caching of Unstructured #153
Introduce manager caching of Unstructured #153
Conversation
As mentioned on Slack, it would be nice to mention in the comments why caching is off by detail (i.e. kubernetes-sigs/controller-runtime#615 (comment)) and why our use of caching won't run into problems. Otherwise lgtm! /ok-to-test |
Also what do you think the risk of this is? Should we delay it to v1.1? |
d451e6b
to
413dda1
Compare
Hmmm, hard to tell. 🤔 We have been caching Unstructured for a long time in our controllers, and there seems to be quite performance gain. But on the other hand, the controller memory consumption might increase. How much depends on the cluster size and how many "object" resource types are configured in HNC. But resources tuning should any cluster-admin be able to do.... I can start by pulling out the "fixes" into a separate PR that can be merged. And leave the Unstructured caching (this PR) open for now? |
Yeah let's punt this to v1.1.
…On Wed, Mar 16, 2022 at 6:55 PM Erik Godding Boye ***@***.***> wrote:
Also what do you think the risk of this is? Should we delay it to v1.1?
Hmmm, hard to tell. 🤔 We have been caching Unstructured for a long time
in our controllers, and there seems to be quite performance gain. But on
the other hand, the controller memory consumption might increase. How much
depends on the cluster since and how many "object" resource types are
configured in HNC. But resources tuning should any cluster-admin be able to
do....
I can start by pulling out the "fixes" into a separate PR that can be
merged. And leave the Unstructured caching (this PR) open for now?
—
Reply to this email directly, view it on GitHub
<#153 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE43PZAG5NT5AHH7I6TXM5LVAJRERANCNFSM5QBODUHA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
/hold for 1.1 |
413dda1
to
7fbbc2a
Compare
Caching of Unstructured is disabled by default in controller-runtime, and that is suboptimal for HNC which is reconciling generic objects configured by users. Tested: Ran both unit-tests ('make test') and integration test ('make test-e2e') successfully.
7fbbc2a
to
4739ff0
Compare
I have now rebased this and fixed commit message and PR description. So should be ready for review - even if we decide to wait for 1.1 with this. /cc @rjbez17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adrianludwin, erikgb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
Caching of Unstructured is disabled by default in controller-runtime, and that is suboptimal for HNC which is reconciling generic objects configured by users.
Tested: Ran both unit-tests ('make test') and integration test ('make test-e2e') successfully.