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

[DISCO-3189] update max age cache control for manifest endpoint #776

Merged
merged 3 commits into from
Jan 30, 2025

Conversation

misaniwere
Copy link
Contributor

@misaniwere misaniwere commented Jan 29, 2025

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

  • This PR conforms to the Contribution Guidelines
  • The PR title starts with the JIRA issue reference, format example [DISCO-####], and has the same title (if applicable)
  • [load test: (abort|skip|warn)] keywords are applied to the last commit message (if applicable)
  • Documentation has been updated (if applicable)
  • Functional and performance test coverage has been expanded and maintained (if applicable)

Sorry, something went wrong.

@misaniwere misaniwere requested a review from a team January 29, 2025 21:35
Copy link
Contributor

@Herraj Herraj left a 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 🤔

@misaniwere
Copy link
Contributor Author

Do we have any tests that assert on this header value 🤔

I don't think so. I'm not sure why that test (test_cron.py) is failing. I saw a similar error on a previous PR but it seemed to magically resolve itself

@tiftran
Copy link
Contributor

tiftran commented Jan 29, 2025

hope you don't mind me dropping by 🙂 . I see that a unit test in test_cron.py is failing again, which seems intermittent, can we file a Jira for it?

@gruberb
Copy link
Member

gruberb commented Jan 29, 2025

Could be that we now have another cron job running (manifest provider). So maybe we have to increase the timeout? But that's just a guess. Happened to me locally once or twice and resolved itself.

@misaniwere
Copy link
Contributor Author

Could be that we now have another cron job running (manifest provider). So maybe we have to increase the timeout? But that's just a guess. Happened to me locally once or twice and resolved itself.

could be, but i wonder why it is inconsistent in different environments. this test has never failed for me locally

@misaniwere
Copy link
Contributor Author

hope you don't mind me dropping by 🙂 . I see that a unit test in test_cron.py is failing again, which seems intermittent, can we file a Jira for it?

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 == [
(
Copy link
Contributor Author

@misaniwere misaniwere Jan 30, 2025

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.

Copy link
Contributor Author

@misaniwere misaniwere Jan 30, 2025

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.

Copy link
Contributor

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 :)

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

@misaniwere misaniwere added this pull request to the merge queue Jan 30, 2025
Merged via the queue into main with commit 0cf6cdb Jan 30, 2025
11 checks passed
@misaniwere misaniwere deleted the DISCO-3189 branch January 30, 2025 15:42
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

4 participants