Improve performance when dealing with thousands of charts #339
Conversation
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
|
retrigger |
1 similar comment
|
retrigger |
Signed-off-by: Ailsa <ailsa.wu@sap.com>
82bdaec to
a64e15b
Compare
|
retrigger |
a64e15b to
8398ab2
Compare
|
retrigger |
3f2b462 to
a188b9b
Compare
|
HI @scbizu , I have made all the check passed, may I have your review one more time ? Appreciate it! |
|
hi @jdolitsky , I made some changes to improve the performance of Chartmuseum. It has worked well for about 3 weeks in our prod system. May I have a review? Hoping it is useful and can be merged to master. Thanks very much. |
Signed-off-by: Ailsa <ailsa.wu@sap.com>
| // It is not crucial if this does not succeed, we will just log any errors | ||
| go server.saveStatefile(log, repo, ir.index.Raw) | ||
| objects := server.getRepoObjectSlice(entry) | ||
| diff := cm_storage.GetObjectSliceDiff(objects, fo.objects, server.TimestampTolerance) |
There was a problem hiding this comment.
Will this method(GetObjectSliceDiff ) refer to the latest improved version?
There was a problem hiding this comment.
Just updated to latest storage release so yes 👍
|
When can this PR be merged and released? As you know, Harbor is depending on Chartmuseum to provide HELM V2 chart management capabilities (still existing together with HELM V3 OCI supporting way for a reasonable time) and some of the Harbor users have met the serious perf issues of managing charts when there are large scale charts at the backend storage. Hope this PR can fix such perf issues of Chartmuseum. Looking forward to its merging and releasing. |
|
Would this PR largely increases the memory that chartmuseum uses since we will maintain a large runtime cache? Should we cache all of the "thousands of" charts ? |
|
will take a look at this ASAP |
Signed-off-by: Ailsa <ailsa.wu@sap.com>
2248e44 to
ed75063
Compare
|
hi @jdolitsky , may I have your review one more time? |
|
@Ailsa-Wu - thanks so much for your patience. Finnally had a chance to look into this, and the change seems to work as expected. Merging. @steven-zou - planning to cut a v0.13.0 release this week (thanks for Hrabor team patience as well) |
|
Please see docs on this new feature: https://github.com/helm/chartmuseum#cache-interval |
|
@jdolitsky hi :-) Do you have any news about planned 0,13 release? I am really anticipating this PR to be part of the next release. Regardless of the answer, I wanted to thank you for your contribution to open source, I really like chartmuseum :-) |
|
@mikhail-putilov - I appreciate the kind words. Will try to get 0.13 out later in this week |
|
@jdolitsky when will v0.13.0 be released? we have been experiencing this issue (#339), therefore heavily interested to upgrade to v0.13.0as it will hopefully solve the problem. thanks |
|
I am interested as well when will the next version be relased? |
Summary
As we have disscsused in the issue, we need to improve the performance when dealing with thousands of charts. Therefor, we made some changes as we proposed before.
Changes
Always read from cache in function getIndexFile() without traversing the files on the disk. If cache is missing, rebuild it
Delta update the cache in all the WRITE requests (e.g.
POST /api/chartsorDELETE /api/charts/)Delta update the cache every 5min (in case the files on the disk are manually manipulated)
fixes #332