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

Bug 1601550 - Enforce disk quota on pending pings directory #1110

Merged
merged 12 commits into from Aug 7, 2020

Conversation

brizental
Copy link
Member

@brizental brizental commented Jul 30, 2020

I am opening this as a draft because I would like to get some opinions on the approach.

Please refer to comments I'll leave in the code to see what I mean.

@brizental brizental requested a review from Dexterp37 Jul 30, 2020
@Dexterp37 Dexterp37 requested review from badboy and removed request for Dexterp37 Jul 30, 2020
@Dexterp37
Copy link
Contributor

Dexterp37 commented Jul 30, 2020

I redirected this to @badboy as I won't be able to get to it today and won't be around tomorrow.

@brizental brizental force-pushed the 1601550-pending-pings-quota branch from f7d1dab to 240257b Compare Jul 30, 2020
glean-core/src/upload/directory.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Show resolved Hide resolved
@brizental brizental force-pushed the 1601550-pending-pings-quota branch 2 times, most recently from 74cdd92 to 5e55a0b Compare Aug 3, 2020
@brizental brizental marked this pull request as ready for review Aug 3, 2020
@brizental brizental requested a review from badboy Aug 3, 2020
@auto-assign auto-assign bot requested a review from Dexterp37 Aug 3, 2020
@Dexterp37 Dexterp37 removed their request for review Aug 3, 2020
@brizental brizental force-pushed the 1601550-pending-pings-quota branch from 980b308 to 50482c3 Compare Aug 3, 2020
Copy link
Member

@badboy badboy left a comment

Still some open questions and some suggestions.
This is on a good way though.

This definitely needs some documentation, e.g. user-facing here: https://mozilla.github.io/glean/book/user/pings/index.html?highlight=rate#limitations

glean-core/metrics.yaml Show resolved Hide resolved
glean-core/src/upload/directory.rs Show resolved Hide resolved
glean-core/src/upload/directory.rs Outdated Show resolved Hide resolved
glean-core/src/upload/directory.rs Outdated Show resolved Hide resolved
glean-core/src/upload/directory.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
glean-core/src/upload/directory.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
@brizental brizental force-pushed the 1601550-pending-pings-quota branch 2 times, most recently from 636c82a to f57b4e1 Compare Aug 5, 2020
@brizental brizental force-pushed the 1601550-pending-pings-quota branch from 7a12c62 to 1c90936 Compare Aug 5, 2020
@brizental brizental requested a review from badboy Aug 6, 2020
badboy
badboy approved these changes Aug 6, 2020
Copy link
Member

@badboy badboy left a comment

Looks good now, I think we can (nearly) land this.
Some inline questions, but nothing totally blocking.

Good work!

docs/user/pings/index.md Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
@brizental brizental requested review from badboy and Dexterp37 and removed request for Dexterp37 Aug 7, 2020
badboy
badboy approved these changes Aug 7, 2020
Copy link
Member

@badboy badboy left a comment

That doesn't look much more complicated.
It's unfortunate that we do the double-reverse, but assuming we're never should have thousands of those I also don't want to over-optimize this just yet.

glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
glean-core/src/upload/mod.rs Outdated Show resolved Hide resolved
brizental and others added 8 commits Aug 7, 2020
Co-authored-by: Jan-Erik Rediger <badboy@archlinux.us>
The problem was that the test was trying to get a request sent
by a worker, but that worker received a PingUploadTask::Wait before
getting a PingUploadTask::Upload and the test was trying to check the
request before it had actually arrived.

This is potentially an issue with more tests, but it only manifested
in this test for now. I opened Bug 1657589 for more in depth
investigation on this.
Co-authored-by: Jan-Erik Rediger <badboy@archlinux.us>
@brizental brizental force-pushed the 1601550-pending-pings-quota branch from 26f2a1f to 0979629 Compare Aug 7, 2020
@brizental brizental merged commit e697751 into mozilla:main Aug 7, 2020
26 of 27 checks passed
@brizental brizental deleted the 1601550-pending-pings-quota branch Aug 7, 2020
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