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

feat: improve guidance for caching support (#451) #482

Merged
merged 4 commits into from
Jan 24, 2019

Conversation

tkrop
Copy link
Member

@tkrop tkrop commented Jan 21, 2019

fixes #451.

The main contributions are in chapters/performance.adoc:173-309 and a small extend in chapters/http-requests.adoc:229-250.

Sorry, that I again mixed some simple reformatting stuff in this commit - especially in chapters/common-headers.adoc. Unfortunately, the diff cannot show this very well.

However, I also mixed an extension to Prefer in chapters/common-headers.adoc:99-155.

@zeitlinger
Copy link
Contributor

finished the review now - using the diff from IDEA fixed the problems of seeing what has changed.

chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/http-requests.adoc Outdated Show resolved Hide resolved
chapters/performance.adoc Show resolved Hide resolved
chapters/performance.adoc Outdated Show resolved Hide resolved
chapters/performance.adoc Outdated Show resolved Hide resolved
chapters/performance.adoc Show resolved Hide resolved
index.adoc Show resolved Hide resolved
@tkrop tkrop force-pushed the 451-improve-guidance-for-cache-support branch from eb324ad to 9c9197f Compare January 24, 2019 13:17
Copy link
Member

@tfrauenstein tfrauenstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx! -- generally fine contribution!
Just 1 non-editorial comment.

chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/common-headers.adoc Outdated Show resolved Hide resolved
chapters/http-requests.adoc Outdated Show resolved Hide resolved
chapters/http-requests.adoc Show resolved Hide resolved
chapters/http-requests.adoc Outdated Show resolved Hide resolved
@tkrop
Copy link
Member Author

tkrop commented Jan 24, 2019

👍

1 similar comment
@tfrauenstein
Copy link
Member

👍

@tfrauenstein tfrauenstein merged commit e44fe86 into master Jan 24, 2019
@maxim-tschumak maxim-tschumak mentioned this pull request Jan 24, 2019
@maxim-tschumak maxim-tschumak deleted the 451-improve-guidance-for-cache-support branch January 24, 2019 16:10
@regispl
Copy link

regispl commented Jan 28, 2019

Hey,

The guidance you're giving sounds good to me in general, but there's a couple of things that 'd like to comment on:

  1. I'm not sure if I understand what's the motivation for the detailed "small vs. medium vs. large data size" guidelines - would it make sense to focus on the "best possible" guidance, rather than providing this detailed explanation on case-to-case basis with only slight differences between them (e.g. guidance for small vs. large is almost identical)? So what I mean is putting together whatever is in the "In general (...)" section plus HEAD support and pagination (which, by the way, is already a "Must" according to other API Guideline's section) and considering it a "baseline"?
    This is even more confusing when you read the "general" section saying that "you should support ETag Together With If-Match / If-None-Match Header on all cacheable endpoints" but then ETags are - for some reason - specifically mentioned one more time for "small", but omitted for "large".
    In general, while the advices are sound and correct, I find this section a bit too detailed and quite difficult to follow.
  2. Following up on the above - I find the "small vs. medium vs. large" distinction very "subjective" and open for interpretation and as such it doesn't seem to be that useful for the readers in the current form. As mentioned above, I'd personally remove it completely (or at least make it way less detailed - one "default" best advice and one or two exceptions listed separately), but if it's to stay, would you consider providing a more detailed guidance on what those mean?
  3. The "Provide efficient methods to warm up and update caches (...)" part makes me think about the "thundering herd" problem on client's startup to me, which always scares me a bit as a service owner. I was wondering if it would make sense to explicitly mention the importance of Rate Limiting here (it's mentioned few paragraphs above in slightly different context)?
  4. Related to the point above: would it make sense to "promote" asynchronous eventing here as a possible alternative for using direct REST API calls & cache for accessing datasets (especially the large ones)? While no silver bullet and not the best option for every single case, when it can be used, the benefits of "notify on change" approach are massive - no periodic GETs, no thundering herds on startup to warm-up cache, no problem when cache expires and the service owning the data is down / slow, no cache "management" (respecting different headers, expiry etc.) at all...

Just a few ideas / doubts that came to my mind when reading this section 👍

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

Successfully merging this pull request may close these issues.

Document use cases and best practices for cache support.
6 participants