refactor(events) using a regular DICT for async events #1748
Conversation
There was a problem hiding this comment.
It is indeed more concise than using lua-resty-lock, which is overkill for what we want to achieve here, so I would say this is a good improvement.
However, why insisting on using the kong.tools.database_cache module? This is clearly not what that module is semantically designed for (a database cache, as per its name) + we can clearly see in the usage being made that we are just wrapping the shm calls.
Less abstraction would be beneficial here - simpler is better - since we can just use the raw cache shm:
-- module level
local dict = ngx.shared.cache -- will also work in CLI as soon as C* v3 PR is merged, in case ever needed
local ok, err = dict:add(key, true, interval - 0.001)
-- ...
local ok, err = dict:incr(AUTOJOIN_RETRIES_KEY, 1)
-- etc...We can then get rid of things like cache.incr() which is doing nothing except wrapping a dict:incr() call... Let's get rid of code we don't need and keep to each module its proper - and originally intended - usage. Our database caching does not need an increment method, it only does so because of this and the rate-limiting plugins using it (while those should use the raw cache shm as well).
In short: database caching should be only concern with serializing and deserializing DB entities.
|
|
||
| local function log_debug(...) | ||
| ngx.log(ngx.DEBUG, "[cluster] ", ...) | ||
| end |
There was a problem hiding this comment.
A better pattern (which is already commonly used across the codebase) is:
local ngx_log = ngx.log
local ERR = ngx.ERR
local WARN = ngx.WARN
local DEBUG = ngx.DEBUG
local function log(lvl, ...)
ngx_log(lvl, "[cluster] ", ...)
end
-- ...
log(ERR, "thing")| local ASYNC_AUTOJOIN_KEY = "events:autojoin" | ||
|
|
||
| local function log_error(...) | ||
| ngx.log(ngx.WARN, "[cluster] ", ...) |
There was a problem hiding this comment.
this says log_error but uses the WARN log level. This is also why the below suggestion is more explicit.
| if not ok then | ||
| return nil, err | ||
| end | ||
| return true |
There was a problem hiding this comment.
cache.rawadd() already returns a boolean. Can be simplified to:
return cache.rawadd(key, true, interval - 0.001)Makes things simpler instead to enforce returning nil, which is honorable but mostly wanted when we are writing our own APIs
| @@ -63,33 +72,35 @@ local function async_autojoin(premature) | |||
| if (autojoin_retries < ASYNC_AUTOJOIN_RETRIES) then | |||
| create_timer(ASYNC_AUTOJOIN_INTERVAL, async_autojoin) | |||
| end | |||
| elseif err ~= "exists" then | |||
| log_error(err) | |||
There was a problem hiding this comment.
The error will be missing some context, maybe better to add a note:
log(ERR, "could not get autojoin lock: ", err)
There was a problem hiding this comment.
Doesn't the error print the file name that generated it and the line number? If it ever appears in the logs, we know where it is.
| elseif #members < 2 then | ||
| -- Trigger auto-join | ||
| local _, err = singletons.serf:autojoin() | ||
| if err then | ||
| ngx_log(ngx.ERR, tostring(err)) | ||
| log_error(tostring(err)) |
There was a problem hiding this comment.
Pretty sure tostring() is not needed anymore now. It's been a few versions that ngx.log accepts table with a __tostring() metamethod (which the DAO is returning).
| elseif elapsed == 0 then | ||
| local ok, err = get_lock() | ||
| if ok then | ||
| log_debug("flushing") |
There was a problem hiding this comment.
needs more context as a user reading this log: flushing what?
| @@ -23,6 +24,10 @@ local function log_error(...) | |||
| ngx.log(ngx.WARN, "[reports] ", ...) | |||
| end | |||
|
|
|||
| local function log_debug(...) | |||
| ngx.log(ngx.DEBUG, "[reports] ", ...) | |||
| end | |||
There was a problem hiding this comment.
ditto: better logging utility idiom
| if elapsed and elapsed == 0 then | ||
| local ok, err = get_lock(ASYNC_AUTOJOIN_KEY, ASYNC_AUTOJOIN_INTERVAL) | ||
| if ok then | ||
| log_debug("auto-joining") |
There was a problem hiding this comment.
ditto: maybe missing some context for a regular user reading this (even with the log prefix)
| if elapsed and elapsed == 0 then | ||
| local ok, err = get_lock(KEEPALIVE_KEY, KEEPALIVE_INTERVAL) | ||
| if ok then | ||
| log_debug("sending keepalive") | ||
| -- Send keepalive |
There was a problem hiding this comment.
Could we get rid of useless comments like this one at the same time? This is obvious + the log added just before states the same as well.
thibaultcha
added
pr/changes requested
| -- worker processes from sending the test request simultaneously. | ||
| -- here we substract the lock expiration time by 1ms to prevent | ||
| -- a race condition with the next timer event. | ||
| return cache.rawadd(KEY, true, PING_INTERVAL - 0.001) |
There was a problem hiding this comment.
We should use the new ngx.shared.kong dict here. It is meant especially for long-lived data that shouldn't get deleted in an LRU fashion (unlike the cache dict which is its purpose).
(Only write to ngx.shared.kong in a safe way with safe_set and safe_add as we want to report no memory errors to the user in that case, so it can be increased).
|
Replaced by #1783, opened against master for 0.9.4 |
This is based on #1748 but adds some cleanup and performance improvements. It gets rid of using the database cache module and uses the raw 'kong' shared dict.
Full changelog