[#200] Respond HTTP 204 when no more changes are available #203

iperdomo · 2020-01-27T11:19:02Z

The proxy server (nginx) will cache this response for 1h. Useful for eager clients re-requesting the same thing over and over again. The value of 1h is arbitrary and we can adjust it.

A client can bypass the proxy by using a request header Pragma: no-cache

The proxy server (nginx) will cache this response for 1h. Useful for _eager_ clients re-requesting the same thing over and over again. The value of 1h is arbitrary and we can adjust it. A client can bypass the proxy by using a request header `Pragma: no-cache`

dlebrero

Good. I like the approach

nginx-auth0/nginx.conf

dlebrero · 2020-01-27T17:37:17Z

nginx-auth0/nginx.conf

+      proxy_cache_lock on;
+      proxy_cache_methods GET;
+      proxy_cache_key $proxy_host$request_uri$akvoemail;
+      proxy_cache_valid 204 60m;


I was thinking more in 30 to 60 secs. "Real time"

For "real time" we have a way to punch a whole and bypass the cache. My intention with the 1h cache is to avoid reaching the database and other services in a transparent way. The client is not aware that we're serving cached content.

nginx-auth0/nginx.conf

dlebrero · 2020-01-27T17:42:51Z

nginx-auth0/nginx.conf

+      proxy_cache_methods GET;
+      proxy_cache_key $proxy_host$request_uri$akvoemail;
+      proxy_cache_valid 204 60m;
+      proxy_cache_bypass $http_pragma;


Same as the comment regarding $request_uri. Allow the client to "cheat". Is it going to be abused?

Assuming that the cache value is 30-60 secs, what is the benefit of using it?

We're not going to document that you can bypass and force to hit the upstream server

Assuming that the cache value is 30-60 secs, what is the benefit of using it?

As mentioned in the previous comment, my intention is to have a larger cache time than 30-60s (for those bad behaving clients) with a escape hatch for our services.

nginx-auth0/nginx.conf

dlebrero · 2020-01-27T17:45:43Z

api/src/clojure/org/akvo/flow_api/endpoint/sync.clj

-                           :next-sync-url (next-sync-url (utils/get-api-root req) alias (:unilog-id changes))}))
+              (if (= offset (unilog/get-cursor db-spec)) ;; end of the log
+                (-> (status {} 204)
+                    (header "Cache-Control" "max-age=3600"))


Isn't the Nginx adding this header? (Haven't tested it)

In any case, there is some duplication here and in the nginx code regarding the caching time. Either we make the proxy_cache_valid follow this header or we remove this header.

According to the documentation, nginx will honor the Cache-Control header from the upstream server. I'll double check what is the behavior when both properties differ.

Isn't the Nginx adding this header? (Haven't tested it)

nginx is not adding the header, but it does cache the HTTP 204 response. I can see the instrumentation header:

< HTTP/1.1 204 No Content < Server: openresty < Date: Tue, 28 Jan 2020 09:24:49 GMT < Connection: keep-alive < Access-Control-Allow-Origin: * < Access-Control-Allow-Credentials: true < Access-Control-Allow-Methods: GET, OPTIONS < Access-Control-Allow-Headers: Accept,Accept-Encoding,Accept-Language,Authorization,Connection,DNT,Host,Origin,Referer,User-Agent < X-Cache-Status: MISS < [...] < HTTP/1.1 204 No Content < Server: openresty < Date: Tue, 28 Jan 2020 09:25:06 GMT < Connection: keep-alive < Access-Control-Allow-Origin: * < Access-Control-Allow-Credentials: true < Access-Control-Allow-Methods: GET, OPTIONS < Access-Control-Allow-Headers: Accept,Accept-Encoding,Accept-Language,Authorization,Connection,DNT,Host,Origin,Referer,User-Agent < X-Cache-Status: HIT

In this case the server is not sending the Cache-Control but we do have the configuration proxy_cache_valid 204 <time>

dlebrero · 2020-01-27T17:56:09Z

api/src/clojure/org/akvo/flow_api/endpoint/sync.clj

-                             (update :data-point-deleted #(map str %)))]
-                (response {:changes (dissoc changes :unilog-id)
-                           :next-sync-url (next-sync-url (utils/get-api-root req) alias (:unilog-id changes))}))
+              (if (= offset (unilog/get-cursor db-spec)) ;; end of the log


This is the third JDBC query: 1 for valid, 1 for last cursor and 1 for reading the data.

Not an issue if we open the DB connection once, but if we open/close for each, maybe we should look into avoiding 2 of those 3 calls.

Being very picky here, so feel free to ignore.

the last cursor is one code path (initial=true)

the valid and reading (next=true), for checking valid and reading data can be transformed into one

dlebrero · 2020-01-27T17:56:48Z

api/src/clojure/org/akvo/flow_api/endpoint/sync.clj

@@ -41,20 +41,24 @@
                db-spec (-> deps :unilog-db :spec (assoc :db-name db-name))


This function is becoming a monster. Maybe time to clean up?

agreed 👍

* Test caching as part of the CI build

* It's quite unlikely that the same user (part of the cache key) is requesting the same URL at the same time

dlebrero

Looks good. I would just ask for removing the “sleep” if it is not a lot of work as it always ends up making the test flaky and slower.

Added a bunch of optional suggestions. Feel free to ignore, no need to justify why.

Thanks!

dlebrero · 2020-01-29T16:29:14Z

api/src/clojure/org/akvo/flow_api/unilog/unilog.clj

+  (fn [request]
+    (let [db-name (get-db-name (:instance-id request))
+          db-spec (-> unilog-db :spec (assoc :db-name db-name) event-log-spec)]
+      (with-open [connection (jdbc/get-connection  db-spec)]


Suggestion: use the with-db-connection macro. The docs discourage using get-connection, plus I guess that using the macro means that we do not need to manually build the {:connection} map

dlebrero · 2020-01-29T16:34:24Z

ci/build.sh

+    ./test.sh "http://localhost:8082/flow/" 2>&1 | grep 'X-Cache-Status: MISS'
+    ./test.sh "http://localhost:8082/flow/" 2>&1 | grep 'X-Cache-Status: HIT'
+    ./test.sh "http://localhost:8082/flow/" 2>&1 | grep 'X-Cache-Status: HIT'
+    ./test.sh "http://localhost:8082/flow/" --header "Pragma: no-cache" 2>&1 | grep 'X-Cache-Status: BYPASS'


Suggestion: I thought we agreed to remove the bypass as we have reduced the cache time to 60 secs, plus we can bypass it by using a random request param

dlebrero · 2020-01-29T16:35:01Z

ci/build.sh


+# Checking nginx caching
+(


Suggestion: Move out of build

dlebrero · 2020-01-29T16:36:42Z

ci/build.sh

+(
+    cd nginx-auth0
+    docker-compose up -d
+    sleep 5


Change: No sleeps in tests please.

did we just deleted the sleep? Why is no longer needed?

it seems that you're checking an outdated code. This checks are not part of the build.sh anymore as you suggested.

Suggestion: Move out of build

dlebrero · 2020-01-29T16:43:01Z

api/src/clojure/org/akvo/flow_api/endpoint/sync.clj

+    (-> (response {:changes (dissoc changes :unilog-id)
+                   :next-sync-url (next-sync-url api-root alias cursor)})
+        (header "Cache-Control" "no-cache"))))
+
 (defn changes [deps {:keys [alias instance-id params] :as req}]
  (let [{:keys [initial cursor next]} (spec/validate-params params-spec params)]
    (if (and initial (or cursor next))


Suggestion: change nested ifs for cond

dlebrero · 2020-01-29T16:43:25Z

api/src/clojure/org/akvo/flow_api/endpoint/sync.clj

@@ -26,35 +26,51 @@
 ;; TODO: Express parameter logic via spec?
 (def params-spec (s/keys :opt-un [::initial ::next ::cursor]))

+(def no-more-changes (-> (status {} 204) (header "Cache-Control" "max-age=60")))
+
+


Suggestion: Remove empty line

dlebrero · 2020-01-29T16:44:41Z

api/src/clojure/org/akvo/flow_api/unilog/unilog.clj

@@ -147,3 +147,14 @@
                             (:form-instances-to-load events-2)))
          data-point-changed (doall (data-point/by-ids ds (:data-point-changed events-2)))]
      (assoc events-2 :form-instance-changed form-instances :data-point-changed data-point-changed))))
+
+


Suggestion: remove empty line 🤐

Instead of creating the connection and then creating a `{:connection conn}` map when needed, we can use the with-db-connection macro

iperdomo · 2020-01-30T10:35:32Z

@dlebrero gave his 👍 via another channel

dlebrero · 2020-01-30T12:14:11Z

I understand now the miscommunication. By “move out” I actually meant “move this code out of the build.sh into another script and call this new script from the build.sh” I did not realized in the code review that we were not doing calling the new script. In general, I would never ever ever suggest removing any automated tests. If I do, I will give a very long explanation of why. Apologies for not being more specific in the first place.

…

Sent from my iPhone

On 30 Jan 2020, at 11:27, Iván Perdomo ***@***.***> wrote: @iperdomo commented on this pull request. In ci/build.sh: > +# Checking nginx caching +( + cd nginx-auth0 + docker-compose up -d + sleep 5 it seems that you're checking an outdated code. This checks are not part of the build.sh anymore as you suggested. Suggestion: Move out of build — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

iperdomo requested a review from dlebrero January 27, 2020 11:19

iperdomo self-assigned this Jan 27, 2020

dlebrero suggested changes Jan 27, 2020

View reviewed changes

iperdomo added 9 commits January 28, 2020 16:10

[#200] Reorder and clean up CI build process

c31a538

[#200] Disallow /sync with Keycloak based authentication

bda49a9

[#200] Use Cache-Control header from upstream to control caching

eb71a36

* Test caching as part of the CI build

[#200] Use a different network name for cache testing

bcfab40

[#200] Decrease cache ttl to 60s

c701ed0

[#200] Use a a single open connection across request

c440b6d

[#200] Add test for X-Cache-Status BYPASS

51bb3eb

[#200] Split changes function

4bcf804

[#200] Increase the values for cache_lock_timeout|cache_lock_age

724a4fb

* It's quite unlikely that the same user (part of the cache key) is requesting the same URL at the same time

iperdomo requested a review from dlebrero January 29, 2020 15:27

dlebrero approved these changes Jan 29, 2020

View reviewed changes

iperdomo added 5 commits January 30, 2020 09:27

[#200] Remove extra empty lines

ab1a29c

[#200] Move test cache to its own script

ebf9416

[#200] Remove Pragma: no-cache capability

7645c5a

[#200] Use with-db-connection macro

b1714c1

Instead of creating the connection and then creating a `{:connection conn}` map when needed, we can use the with-db-connection macro

[#200] Refactor nested (if) to (cond)

aa5407e

iperdomo requested a review from dlebrero January 30, 2020 09:31

iperdomo merged commit e69a16a into develop Jan 30, 2020

iperdomo deleted the issue/200-caching branch January 30, 2020 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#200] Respond HTTP 204 when no more changes are available #203

[#200] Respond HTTP 204 when no more changes are available #203

iperdomo commented Jan 27, 2020

dlebrero left a comment

dlebrero Jan 27, 2020

iperdomo Jan 28, 2020

dlebrero Jan 27, 2020

iperdomo Jan 28, 2020

iperdomo Jan 28, 2020

dlebrero Jan 27, 2020

iperdomo Jan 28, 2020

iperdomo Jan 28, 2020

dlebrero Jan 27, 2020

iperdomo Jan 28, 2020

dlebrero Jan 27, 2020

iperdomo Jan 28, 2020

dlebrero left a comment

dlebrero Jan 29, 2020

dlebrero Jan 29, 2020

dlebrero Jan 29, 2020

dlebrero Jan 29, 2020

dlebrero Jan 30, 2020

iperdomo Jan 30, 2020

dlebrero Jan 29, 2020

dlebrero Jan 29, 2020

dlebrero Jan 29, 2020

iperdomo commented Jan 30, 2020

dlebrero commented Jan 30, 2020 via email

		@@ -41,20 +41,24 @@
		db-spec (-> deps :unilog-db :spec (assoc :db-name db-name))

[#200] Respond HTTP 204 when no more changes are available #203

[#200] Respond HTTP 204 when no more changes are available #203

Conversation

iperdomo commented Jan 27, 2020

dlebrero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlebrero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iperdomo commented Jan 30, 2020

dlebrero commented Jan 30, 2020 via email