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
Reduce Carvel response times for GetAvailablePackageSummaries #4378
Conversation
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@@ -25,15 +25,9 @@ import ( | |||
log "k8s.io/klog/v2" | |||
) | |||
|
|||
func (s *Server) buildAvailablePackageSummary(pkgMetadata *datapackagingv1alpha1.PackageMetadata, pkgVersionsMap map[string][]pkgSemver, cluster string) (*corev1.AvailablePackageSummary, error) { | |||
func (s *Server) buildAvailablePackageSummary(pkgMetadata *datapackagingv1alpha1.PackageMetadata, latestVersion string, cluster string) *corev1.AvailablePackageSummary { |
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.
There was no requirement for having the package map, just the latest version, which also means we don't need the ability to return an error here.
// getPkgs requests the packages for the given cluster and namespace and sends | ||
// them to the channel to be processed immediately, closing the channel | ||
// when finished or when an error is returned. | ||
func (s *Server) getPkgs(ctx context.Context, cluster, namespace string, ch chan<- *datapackagingv1alpha1.Package) error { |
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.
This function wasn't actually used at all, so I updated it to do what I want. Importantly, it does not iterate all the results into an in-memory slice, but rather sends each package down the return channel as it iterates (so we can process them while it continues iterating).
The other important point here is, as mentioned in the comment, that this function guarantees to close the channel, so we can be sure when it has completed (or returned with an error).
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
// Collect the packages for a particular refName to be able to send the | ||
// latest semver version. For the moment, kapp-controller just returns | ||
// CRs with the default alpha sorting of the CR name. | ||
// Ref https://kubernetes.slack.com/archives/CH8KCCKA5/p1646285201181119 |
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.
Great, thanks for mentioning it. Interesting discussion!
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.
Excelsior! Thanks for the changes, the single request+ulterior processing in channel is a great approach.
We should release soon including this change, so that users can effectively/painlessly use carvel packages :P
That is mind-blowing!!! |
Great, thanks. I'll try to update the get install pkg function soon too. |
Description of the change
See the comment on the related issue for the background investigation.
This PR reduces the response time for the GetAvailablePackageSummaries RPC call of the Carvel plugin with the TCE repository (12 package metas, 18 packages) from over 5s to around 280-500ms on my local computer, without changing the functionality (
although I've left a TODO related to one assumption which may not be correct, but it's an easy update if the assumption is falseEdit: I've now removed this assumption).Timing with TCE repo after updating without assumption of version order:
I'm keen to test it with a much larger repo.Edit: Improvements are also 10-fold for my local system with over 100 packages, from 40-50s down to 4-5s. See results on the issue.Benefits
Faster catalog when using the Carvel support.
Possible drawbacks
Applicable issues
Additional information
If we're happy with this approach, we can apply the same to other parts of the carvel support (installed package summaries).
I'll leave some notes inline too.