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

Caching retrieveClansSummaries responses #56

Merged
merged 21 commits into from
Sep 11, 2019
Merged

Caching retrieveClansSummaries responses #56

merged 21 commits into from
Sep 11, 2019

Conversation

matheuscscp
Copy link
Contributor

@matheuscscp matheuscscp commented Sep 10, 2019

Problem: retrieveClansSummaries fetches a lot of data for many clans in most use cases (clan rankings). On a load test we detected that database CPU usage reaches 100% with a small load (compared to the load we expect for the launch of our next game).

Solution: Cache clans summaries individually, because players are used to rankings with information a little bit delayed.

@coveralls
Copy link

coveralls commented Sep 10, 2019

Coverage Status

Coverage increased (+0.4%) to 62.118% when pulling fd2e3fa on matheuscscp:feature/clans-summaries-cache into 80586f6 on topfreegames:master.

Copy link

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

I've done a first pass, let me know your thoughts. Bear in mind that this is the first time I've looked at this code, so there's a lot of context that I'm missing.

api/app.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
caches/clan_test.go Outdated Show resolved Hide resolved
config/default.yaml Show resolved Hide resolved
testing/helpers.go Outdated Show resolved Hide resolved
caches/clan_test.go Show resolved Hide resolved
testing/helpers.go Outdated Show resolved Hide resolved
@codingllama
Copy link

Please add a description to the PR explaining what the problem is and how we're attempting to solve it.

@matheuscscp
Copy link
Contributor Author

Please add a description to the PR explaining what the problem is and how we're attempting to solve it.

Done!

caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
Copy link

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Thanks for the changes and for the added PR description, @matheuscscp.

caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
testing/helpers.go Show resolved Hide resolved
caches/clan_test.go Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
api/app.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
caches/clan.go Outdated Show resolved Hide resolved
Copy link

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

LGTM (Looks Good To Me).

Please wait for Camila's approval, but otherwise feel free to merge. 👍

caches/clan.go Outdated Show resolved Hide resolved
caches/clan_test.go Outdated Show resolved Hide resolved
caches/clan_test.go Show resolved Hide resolved
@cscatolini
Copy link
Contributor

Feel free to merge if you think this is ready or once you've incorporated the latest suggestions by Alan.

@matheuscscp matheuscscp merged commit 8122b8a into topfreegames:master Sep 11, 2019
@matheuscscp matheuscscp deleted the feature/clans-summaries-cache branch September 11, 2019 19:07
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.

4 participants