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

Add a caching layer to .well-known responses #4516

Merged
merged 3 commits into from Jan 30, 2019

Conversation

3 participants
@richvdh
Copy link
Member

richvdh commented Jan 29, 2019

No description provided.

@richvdh richvdh requested a review from matrix-org/synapse-core Jan 29, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #4516 into develop will decrease coverage by 0.02%.
The diff coverage is 87.9%.

@@             Coverage Diff             @@
##           develop    #4516      +/-   ##
===========================================
- Coverage    74.75%   74.73%   -0.03%     
===========================================
  Files          336      337       +1     
  Lines        34219    34416     +197     
  Branches      5570     5608      +38     
===========================================
+ Hits         25580    25720     +140     
- Misses        7060     7108      +48     
- Partials      1579     1588       +9
@erikjohnston
Copy link
Member

erikjohnston left a comment

Looks broadly sane. Shame that we end up with yet another cache implementation

cache_period = WELL_KNOWN_DEFAULT_CACHE_PERIOD
else:
cache_period = min(cache_period, WELL_KNOWN_MAX_CACHE_PERIOD)

This comment has been minimized.

@erikjohnston

erikjohnston Jan 29, 2019

Member

Don't we also want a min cache period here here?

This comment has been minimized.

@richvdh

richvdh Jan 30, 2019

Author Member

Not sure, tbh. What would you set it to? If someone says "don't cache", I'm minded to respect that.

This comment has been minimized.

@erikjohnston

erikjohnston Jan 30, 2019

Member

Oh, I hallucinated you adding a MIN constant. I don't have strong feelings either way, though I wouldn't be surprised if people used max_age=0, but we can revisit it later if it proves a problem

else:
k, v = directive, None
k = k.lower()
cache_controls[k] = v

This comment has been minimized.

@erikjohnston

erikjohnston Jan 29, 2019

Member

I think this needs some white space stripping? It'd be nice to have some tests that have multiple values.

Some random examples from the internet:

  • Cache-Control: private; max-age=0 (hacker news, looks bogus with the semi colon?)
  • Cache-Control: max-age=0, private, must-revalidate (github.com)
  • cache-control: private, max-age=0 (google.com)

This comment has been minimized.

@richvdh

richvdh Jan 30, 2019

Author Member

thanks. fixed.

@richvdh richvdh added this to To Do in Homeserver Task Board via automation Jan 29, 2019

@richvdh richvdh moved this from To Do to In progress in Homeserver Task Board Jan 29, 2019

richvdh added some commits Jan 30, 2019

Add some jitter to the default cache period
otherwise we get a stampede every hour.
@richvdh

This comment has been minimized.

Copy link
Member Author

richvdh commented Jan 30, 2019

[CI failed due to flake8 update]

@richvdh richvdh merged commit bc5f6e1 into develop Jan 30, 2019

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details

@richvdh richvdh moved this from In progress to Done in Homeserver Task Board Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment