Merge branch 'sh-expand-on-definition-of-done' into 'master'

Add performance/scalability concerns to CONTRIBUTING.md

See merge request !8677

2 files changed, 49 insertions, 0 deletions

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 1fd29fef4f0..ae143c58290 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -399,6 +399,12 @@ There are a few rules to get your merge request accepted:
 1. Contains functionality we think other users will benefit from too
 1. Doesn't add configuration options or settings options since they complicate
    making and testing future changes
+1. Changes do not adversely degrade performance.
+   - Avoid repeated polling of endpoints that require a significant amount of overhead
+   - Check for N+1 queries via the SQL log or [`QueryRecorder`](https://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
+   - Avoid repeated access of filesystem
+1. If you need polling to support real-time features, please use
+   [polling with ETag caching][polling-etag].
 1. Changes after submitting the merge request should be in separate commits
    (no squashing). If necessary, you will be asked to squash when the review is
    over, before merging.
@@ -434,6 +440,7 @@ the feature you contribute through all of these steps.
 1. Description explaining the relevancy (see following item)
 1. Working and clean code that is commented where needed
 1. Unit and integration tests that pass on the CI server
+1. Performance/scalability implications have been considered, addressed, and tested
 1. [Documented][doc-styleguide] in the /doc directory
 1. Changelog entry added
 1. Reviewed and any concerns are addressed
@@ -540,6 +547,7 @@ available at [http://contributor-covenant.org/version/1/1/0/](http://contributor
 [UX Guide for GitLab]: http://docs.gitlab.com/ce/development/ux_guide/
 [license-finder-doc]: doc/development/licensing.md
 [GitLab Inc engineering workflow]: https://about.gitlab.com/handbook/engineering/workflow/#labelling-issues
+[polling-etag]: https://docs.gitlab.com/ce/development/polling.html
 
 [^1]: Specs other than JavaScript specs are considered backend code. Haml
       changes are considered backend code if they include Ruby code other than just
diff --git a/doc/development/polling.md b/doc/development/polling.md
new file mode 100644
index 00000000000..a086aca6697
--- /dev/null
+++ b/doc/development/polling.md
@@ -0,0 +1,41 @@
+# Polling with ETag caching
+
+Polling for changes (repeatedly asking server if there are any new changes)
+introduces high load on a GitLab instance, because it usually requires
+executing at least a few SQL queries. This makes scaling large GitLab
+instances (like GitLab.com) very difficult so we do not allow adding new
+features that require polling and hit the database.
+
+Instead you should use polling mechanism with ETag caching in Redis.
+
+## How to use it
+
+1. Add the path of the endpoint which you want to poll to
+   `Gitlab::EtagCaching::Middleware`.
+1. Implement cache invalidation for the path of your endpoint using
+   `Gitlab::EtagCaching::Store`. Whenever a resource changes you
+   have to invalidate the ETag for the path that depends on this
+   resource.
+1. Check that the mechanism works:
+   - requests should return status code 304
+   - there should be no SQL queries logged in `log/development.log`
+
+## How it works
+
+1. Whenever a resource changes we generate a random value and store it in
+   Redis.
+1. When a client makes a request we set the `ETag` response header to the value
+   from Redis.
+1. The client caches the response (client-side caching) and sends the ETag as
+   the `If-None-Modified` header with every subsequent request for the same
+   resource.
+1. If the `If-None-Modified` header matches the current value in Redis we know
+   that the resource did not change so we can send 304 response immediately,
+   without querying the database at all. The client's browser will use the
+   cached response.
+1. If the `If-None-Modified` header does not match the current value in Redis
+   we have to generate a new response, because the resource changed.
+
+For more information see:
+- [RFC 7232](https://tools.ietf.org/html/rfc7232)
+- [ETag proposal](https://gitlab.com/gitlab-org/gitlab-ce/issues/26926)