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

Optimize like and download queries for models and worlds #16

Merged
merged 4 commits into from
Apr 14, 2023

Conversation

AlejoAsd
Copy link
Collaborator

@AlejoAsd AlejoAsd commented Apr 4, 2023

Queries used to update like and download counts for models and worlds have been updated to use SQL expressions instead of computing and updating values inside Fuel. This change improves like endpoint performance and also removes the considerable cpu spike when downloading models.

One important note is that using an expression to update the value does not return the total like/download count. This value was previously returned by the endpoints and this is no longer the case. I will double check that this does not impact our app client.

Update model and worlds like handlers to not return total likes.
@AlejoAsd AlejoAsd self-assigned this Apr 4, 2023
@german-e-mas
Copy link
Collaborator

One important note is that using an expression to update the value does not return the total like/download count. This value was previously returned by the endpoints and this is no longer the case. I will double check that this does not impact our app client.

Thanks! From the top of my head some changes will be necessary.

bundles/worlds/worlds_service.go Outdated Show resolved Hide resolved
handler_models.go Show resolved Hide resolved
@AlejoAsd AlejoAsd requested a review from marcoshuck April 5, 2023 14:21
@marcoshuck
Copy link
Collaborator

Thanks for the changes Alejo. We should make the test job pass in order to merge this PR. Other than that, this looks good to be merged.

@AlejoAsd
Copy link
Collaborator Author

AlejoAsd commented Apr 5, 2023

Something else broke tests. I've run tests on main and they are having the same issues as this PR. Will create a separate PR to fix tests, merge into this, and then merge this.

Edit: this was due to an issue on my local setup. Tests failing are in fact due to changes made in this PR. Will fix tests here instead.

@marcoshuck
Copy link
Collaborator

Something else broke tests. I've run tests on main and they are having the same issues as this PR. Will create a separate PR to fix tests, merge into this, and then merge this.

Got it, thanks. Let me know if there's anything I can do to help.

Add delta parameter to increase/decrease like and download methods.

Signed-off-by: Alejo Carballude <alejocarballude@gmail.com>
Signed-off-by: Alejo Carballude <alejocarballude@gmail.com>
Signed-off-by: Alejo Carballude <alejocarballude@gmail.com>
@AlejoAsd AlejoAsd merged commit 96163db into main Apr 14, 2023
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.

None yet

3 participants