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

Add last modified support #8

Merged

Conversation

ties
Copy link
Collaborator

@ties ties commented Jun 18, 2021

Adds Last-Modified header support and enables this (and ETags)
by default for rtrmon.

I'm using rtrmon with a backend that suports If-Modified-Since
but not If-None-Match. To poll this often (sub-minute interval)
it is better to use conditional requests.

I am not sure if it makes sense to enable this by default for stayrtr. My understanding is that the combination of both headers should be handled properly by back-ends (and are sent by browsers) - so it should be safe to do.

Furthermore I am not a golang expert, so this change is a bit mechanical, let me know what you think.

ties added 2 commits June 18, 2021 15:11
Adds `Last-Modified` header support and enables this (and `ETag`s)
by default for rtrmon.

I'm using rtrmon with a backend that suports `If-Modified-Since`
but not `If-None-Match`. To poll this often (sub-minute interval)
it is better to use conditional requests.
@job
Copy link
Member

job commented Jun 18, 2021

@ties thanks! Conditional requests are very useful indeed!

You indicate that your backend does not support eTags, which probably means the change is harder to test. Perhaps it makes sense to split this into two aspects?

  1. introduce support for if-modified-since: (which is a simpler mechanism, and you can test)
  2. introduce support for eTags

@ties
Copy link
Collaborator Author

ties commented Jun 18, 2021

@job I agree that is is good to be careful about this.

The current status is:

  • stayrtr uses ETags, and
  • rtrmon does not use ETags nor if-modified-since:.

Because it is already used in stayrtr, I think enabling ETags on rtrmon is safe. The HTTP(S) endpoints that connects to are the same as stayrtr already connects to.

I can test rtrmon against endpoints that support only if-modified-since:. How about this: If the code looks good, both headers are enabled in rtrmon by default (since I think two toggles is not needed). if-modified-since: gets disabled in stayrtr by default for now (since I won't be actively running that), and may be set to default-enabled later.

@job
Copy link
Member

job commented Jun 18, 2021

Hi @ties

You wrote:

if-modified-since: gets disabled in stayrtr by default

Did you mean to say "If-none-match gets disabled in stayrtr by default" ?

@ties
Copy link
Collaborator Author

ties commented Jun 18, 2021

Hi @job

Did you mean to say "If-none-match gets disabled in stayrtr by default" ?

I think it is already enabled by default. The new part (if-modified-since) would be disabled by default (and the flag is intentionally named last.modified after the server-side header for consistency).

@randomthingsandstuff randomthingsandstuff merged commit 97ad4f4 into bgp:master Jun 18, 2021
@randomthingsandstuff
Copy link
Contributor

Nice clean commits. Thanks for the contrib!

@ties ties deleted the feature/rtrmon-add-last-modified-support branch June 18, 2021 21:44
@ties
Copy link
Collaborator Author

ties commented Jun 21, 2021

I think it was safe to merge. Thanks for handling this quickly :).

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.

3 participants