-
Notifications
You must be signed in to change notification settings - Fork 10
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
[DISCO-3189] update max age cache control for manifest endpoint #776
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any tests that assert on this header value 🤔
I don't think so. I'm not sure why that test ( |
hope you don't mind me dropping by 🙂 . I see that a unit test in |
Could be that we now have another cron job running ( |
could be, but i wonder why it is inconsistent in different environments. this test has never failed for me locally |
We could, it is blocking this work though. Thankfully this isn't urgent. The issue seems quite hard to reproduce. Are you suggesting we file a ticket, and keep this PR on hold till the ticket is done? Or figure out a band-aid fix for now and then work on the ticket later? Open to either or any other ideas |
# Verify log messages for the different branches | ||
assert caplog.record_tuples == [ | ||
assert cron_logs == [ | ||
( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i pushed a fix, hopefully this solves the issue. Apparently caplog.record_tuples
captures all log messages (including third-party logs), not just those from the cron module and so it seemed to be capturing an additional warning log happening elsewhere, causing a mismatch. I added this to filter out and focus on only the logs we're concerned with for this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every now and then it would capture this warning:
('google.auth.compute_engine._metadata', 30, 'Compute Engine Metadata server unavailable on attempt 2 of 3. Reason: timed ' 'out')
which made the assertion fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, it is blocking this work though. Thankfully this isn't urgent. The issue seems quite hard to reproduce. Are you suggesting we file a ticket, and keep this PR on hold till the ticket is done? Or figure out a band-aid fix for now and then work on the ticket later? Open to either or any other ideas
I see that you merged it LOL. I wasn't saying we should block this work, just that we should record it somewhere to keep track of it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, do you still want me to keep track of it? I merged a fix which seems to have worked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth a short investigation to know whether we should worry about it. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it can't hurt. I'll file a ticket for it
References
JIRA: DISCO-3189
Description
A slight performance improvement - since we only fetch new data every 24 hours, we can afford to serve cached data for longer. This PR increases the max age in the Cache Control header from 5 mins to 8 hours.
PR Review Checklist
Put an
x
in the boxes that apply[DISCO-####]
, and has the same title (if applicable)[load test: (abort|skip|warn)]
keywords are applied to the last commit message (if applicable)