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.

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

Remove RawIcon from chart versions #1460

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Jan 16, 2020

Description of the change

Removes the RawIcon field from the json/bson responses.

I noticed while testing today that listing the (default) catalog is a 6M payload:
catalog-6MB

and looking at a specific wordpress chart is as 14M payload:
wordpress-versions-14MB

A large part of this is because we are now including the RawIcon data as part of the responses (for each chart, or for each version of a chart).

Benefits

The request for the list of charts (default repos) goes down from 6M to 3M. The request for a chart's versions goes down from 14M to 4M.

Other info

I think this issue (of the huge payloads) appears to have begun with the switch away from the monocular repo, as I notice there that the chart model is explicitly defined to not serialize certain fields to (and from) json, including the RawIcon and ChartVersions:

https://github.com/helm/monocular/blob/master/cmd/chartsvc/models/chart.go#L42

It appears that the postgres version is dependent on these being serialized - interested to know the background here.

I think a better solution than what I've tested here would be, as per the TODO, to store the raw icon not in the json column, but a separate column, so it's easy to pull out for serving, but is not included in the normal payload.

Thoughts?

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

I am okay with this workaround. It's true that the MongoDB driver filters out the raw_icon when requesting a ChartVersion so we could do this in the getChartVersion method but here is okay.

I agree with your reasoning that the icon should be in a different column (it should even be in a different table: files instead of charts). I also had a note to store the icon as a byte array (instead of encoding it in base64 which I assume is worse) but I couldn't do so because it's stored in the JSON object right now.

Let me open a new issue so we can tackle this separately (I don't think we need to block the release because we have this fix).

@andresmgot andresmgot merged commit 158da32 into vmware-tanzu:master Jan 16, 2020
@absoludity
Copy link
Contributor Author

Note that this workaround improves the payloads slightly, but they are still megabytes with lots of redundant data. I'm keen to understand why the json annotations for the chart model went from - in monocular (ie. don't serialize at all, for raw_icon and chart_versions etc) to being included in the current Kubeapps model annotations?

@andresmgot
Copy link
Contributor

Note that this workaround improves the payloads slightly, but they are still megabytes with lots of redundant data. I'm keen to understand why the json annotations for the chart model went from - in monocular (ie. don't serialize at all, for raw_icon and chart_versions etc) to being included in the current Kubeapps model annotations?

When using mongodb, it's using the bson annotation. This is useful when using the mgo library to get data. That's not possible with PostgreSQL so we are using the JSON annotation. We would need to manually remove it as you have done in this PR when serializing it as JSON.

I think that answer your question but I may be missing something. Let me know otherwise!

@absoludity
Copy link
Contributor Author

Thanks - it answers for RawIcon and IconContentType, but not ChartVersions, which doesn't have a bson annotation either in monocular:

https://github.com/helm/monocular/blob/master/cmd/chartsvc/models/chart.go#L42

@andresmgot
Copy link
Contributor

andresmgot commented Jan 17, 2020

Thanks - it answers for RawIcon and IconContentType, but not ChartVersions, which doesn't have a bson annotation either in monocular:

https://github.com/helm/monocular/blob/master/cmd/chartsvc/models/chart.go#L42

The reason is the same, since the annotation is not bson: "-" it's parsing it by default (not ignoring it).

You are correct though, the chartsvc is also skipping the ChartVersions in those requests. I am going to send a follow up PR for this so both requests are the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants