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

fix: storage used should not count failed uploads #1430

Merged
merged 4 commits into from
Jun 15, 2022

Conversation

flea89
Copy link
Contributor

@flea89 flea89 commented Jun 8, 2022

Resolves #1198

@flea89 flea89 changed the title Fix/user upload storage used fix: storage used should not count failed uploads Jun 8, 2022
@flea89 flea89 marked this pull request as ready for review June 14, 2022 09:20
@adamalton adamalton self-assigned this Jun 14, 2022
AND u.deleted_at is null
AND p.status = 'Pinned'
GROUP BY c.cid,
c.dag_size
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we group by c.dag_size here? Will that not mean that if two files have the same dag size that one of them will be ignored?

I tried removing this line, and the tests still pass. Should we remove it?

Copy link
Contributor Author

@flea89 flea89 Jun 15, 2022

Choose a reason for hiding this comment

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

c.dag_size is used in the SELECT, so I'm pretty sure the expression need to be in the group_by.
I'm not sure why the tests were not failing for you 🤔, you should get column "c.dag_size" must appear in the GROUP BY clause or be used in an aggregate function.

Will that not mean that if two files have the same dag size that one of them will be ignored?

I don't think. The dag_size is a column on content and we're already grouping on the primary key cid so that adding the dag_size shouldn't really changed the returned rows but rather just make it available to the aggreate SUM.

@adamalton
Copy link
Contributor

Other than my one comment above, I think this all looks good.

…nned-only.sql

Co-authored-by: Adam Alton <adamalton@gmail.com>
@joshJarr joshJarr self-requested a review June 15, 2022 12:28
@flea89 flea89 merged commit a86d7e2 into main Jun 15, 2022
@flea89 flea89 deleted the fix/user-upload-storage-used branch June 15, 2022 13:36
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.

Users uploaded storage used should only count content that is pinned
3 participants