Make 3scale batching policy compatible with the caching policy #757
Conversation
davidor
force-pushed
the
batching-policy-combined-with-caching-policy
branch
3 times, most recently
from
c616be6 to
90f498e
Compare
davidor
changed the title
|
|
||
| -- Note: "update_func" should be one of the handlers exposed by the caching | ||
| -- policy. | ||
| function _M:update(transaction, backend_status, update_func) |
There was a problem hiding this comment.
I'll move update_func to the initializer so it becomes an attr of the class.
There was a problem hiding this comment.
What about not passing update_func at all?
It is passed here only to be called. This function just gets keys for a transaction.
Actually this class feels a bit weird as it has several collaborators, but they are grouped only by one property: they need keys for the transaction. One reads from shdict and other is a function to be executed. Seems to me they don't really belong to be together.
There was a problem hiding this comment.
You're right. I was having second thoughts about this class when I opened this PR, and that's why I added the comment. The objective was to isolate the management of the backend downtime cache, but we need to use a handler stored in the context and that makes it a bit weird.
In the end, I left the cache handling responsibility in the main class of the policy. I think that's better for now.
| @@ -136,6 +164,9 @@ function _M:access(context) | |||
|
|
|||
| ensure_report_timer_on(self, service_id, backend) | |||
|
|
|||
| -- The caching policy sets a cache handler in the context | |||
| self.cache_handler = context.cache_handler | |||
There was a problem hiding this comment.
Ideally, the cache handler should be set in the initializer and passed to BackendDowntimeCache.new(). Unfortunately, that's not possible because the exported cache_handler is not available there.
There was a problem hiding this comment.
Is it necessary to mutate self ? I know right now each service will get own copy the policy, but that might not be necessary in the future.
I'd like to keep policies "multi tenant" in a way they could operate with the same configuration on multiple services.
| local cached = self.cache_handler and | ||
| self.backend_downtime_cache:get(transaction) | ||
|
|
||
| if not cached or cached ~= 200 then |
There was a problem hiding this comment.
Might be better to do first the positive check ? if cached == 200 then ... else ... end
There was a problem hiding this comment.
Changed. Looks better 👍
I was thinking in the case of cached being a table, that's why I added that nil check, but it's an integer, so there's no need to do that.
…e context when backend is down
Seems to be needed with the new version of OpenResty.
davidor
force-pushed
the
batching-policy-combined-with-caching-policy
branch
from
2ce1533 to
4497c73
Compare
| @@ -5,7 +5,7 @@ local concat = table.concat | |||
| local sort = table.sort | |||
| local unpack = table.unpack | |||
| local ngx_re = ngx.re | |||
| local table = table | |||
| local new_tab = require('resty.core.base').new_tab | |||
There was a problem hiding this comment.
@mikz I remember that we discussed this in a previous PR.
I needed to require this in order to make the test pass. I guess it has to do with the OpenResty upgrade that we merged today.
| local function handle_backend_ok(self, transaction) | ||
| local function update_downtime_cache(cache, transaction, backend_status, cache_handler) | ||
| local key = keys_helper.key_for_cached_auth(transaction) | ||
| cache_handler(cache, key, backend_status) |
There was a problem hiding this comment.
Isn't this handler being called twice ? Once by APIcast policy and once by this one?
There was a problem hiding this comment.
No, because when this policy is enabled, APIcast does not authorize nor report to the 3scale backend. And the handler is only called when there's a backend response to be handled.
Closes #738