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(db) implement ttl support for kong.db and new admin api #3638

Merged
merged 5 commits into from Aug 9, 2018

Conversation

Projects
None yet
2 participants
@bungle
Copy link
Member

bungle commented Jul 20, 2018

Summary

  • Implements TTL support in Cassandra strategy
  • Implements TTL support in Postgres strategy
  • Implements TTL support in Generic DAO
  • Implements TTL support in Admin API Endpoints
  • Implements TTL cleanup timer for Postgres strategy
  • Renames kong.db.init_connector to kong.db.init

@bungle bungle force-pushed the feat/db-ttl branch from c2f8f9b to aabd765 Jul 20, 2018

@bungle bungle force-pushed the feat/db-ttl branch 4 times, most recently from d8e0805 to 2fbb2e1 Jul 20, 2018

@bungle bungle removed the pr/do not merge label Jul 21, 2018

@bungle bungle force-pushed the feat/db-ttl branch from 2fbb2e1 to 6d5e371 Jul 21, 2018

@thibaultcha
Copy link
Member

thibaultcha left a comment

Only had time to review parts of the PostgreSQL strategy commit for now!

fields.updated_at and
fields.updated_at.timestamp and
fields.updated_at.auto then
ttl_value = escape_literal(connector, ttl_value + attributes.updated_at, fields.ttl)

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Jul 25, 2018

Member

Why is the TTL value derived from updated_at? Shouldn't a TTL always be derived form the creation date?

This comment has been minimized.

Copy link
@bungle

bungle Jul 27, 2018

Author Member

If you want to patch entity a new ttl we use updated_at If present. You can also remove ttl (ttl=0) with patch.

fields.updated_at.auto then
ttl_value = escape_literal(connector, ttl_value + attributes.updated_at, fields.ttl)

else

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Jul 25, 2018

Member

Should this be elseif not update then? Not sure, but we'd rather avoid touching the TTL if we are updating a row without editing its created_at field, right? Unless I missed something that already prevents that, or that makes this behaviour desirable.

This comment has been minimized.

Copy link
@bungle

bungle Jul 27, 2018

Author Member

We won't touch ttl If not requested on updates (there is that if ttl...).

for i = 1, argc do
local name = argn[i]
local value
local update = options and options.update

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Jul 25, 2018

Member

my 2cts: is_update (which you previously came up with) was more readable 😅

@bungle bungle force-pushed the feat/db-ttl branch 12 times, most recently from 8fe7c31 to aea798b Aug 6, 2018

for _, sub_option in ipairs(sorted_keys(option_errors)) do
len = len + 1
buf[len] = fmt("%s.%s: %s", option_name, sub_option,
option_errors[sub_option])

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 8, 2018

Member

style: many places in this PR do this for line wrapping:

local _ = fmt("%s.%s: %s", option_name, 
  option_errors[sub_option])

When in fact, our code style asks to do this:

local _ = fmt("%s.%s: %s", option_name, 
              option_errors[sub_option])

This comment has been minimized.

Copy link
@bungle

bungle Aug 9, 2018

Author Member

Yes, I'll try to correct that and somehow setup the IDE to do that for me.

@@ -111,7 +111,7 @@ local function prefix_err(self, err)
end


function DB:init_connector()
function DB:init()

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 8, 2018

Member

If there are no functional change to this method and its responsibilities, I don't think we should rename it. init_connector clearly shows the intent to initiate a connection to the dabase, while init isn't really clear. Especially now that we introduce init_worker(), does it mean that init() is meant to be invoked in init_by_lua_*? It isn't, so the renaming isn't justified imho.

This comment has been minimized.

Copy link
@bungle

bungle Aug 9, 2018

Author Member

Ok, I will remove:
83a556e

The original intent was that we have these:

  1. kong.db.connector:init()
  2. kong.db.init() (it calls also the connector:init() — meant to be used in init phase)
  3. kong.db.init_worker() — (it calls also the connector:init_worker() — meant to be used in init_worker phase)

Just to make kong.db follow similar patterns as we use elsewhere (and in old DAO). And I though it was nicer to think that kong.db.init was not called than kong.db.init_connector was not called. And as we already had direct way to init connector via kong.db.connector:init() (same number of chars as with init_connector), I though well lets add init phase handler to db too. You never know if we only need to init connector or do other stuff as well, and if yes, later on we can find ourselves doing this:

local db = kong.db.new()
db:init_connector()
db:init()

vs. just:

local db = kong.db.new()
db:init()

Though even that has been argued that should the .new do it all, or perhaps have different constructors or parameters to new that also does initialization.

So the init_worker was missing, but we had:

  1. kong.db.connector:init()
  2. kong.db.init_connector()

Those are pretty much equal. There is only a bit of error handling in init_connector.

Then I needed init_worker in connector. Should I add:

  1. kong.db.init_worker_connector OR
  2. kong.db.init_connector_worker OR
  3. kong.db.init_worker

But it looks like what you sugges is to just have:

kong.db.init_connector
kong.db.init_worker

So I will be making those changes.

@@ -141,7 +141,6 @@ local meta_errors = {
FIELDS_KEY = "each key in fields must be a string",
ENDPOINT_KEY = "value must be a field name",
TTL_RESERVED = "ttl is a reserved field name when ttl is enabled",
TTL_CREATED_AT = "ttl can only be enabled on entities that have a 'created_at' timestamp field",

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 8, 2018

Member

Why are we removing this constraint? Mostly curious

This comment has been minimized.

Copy link
@bungle

bungle Aug 9, 2018

Author Member

At first I thought I need that when I created PR for ttl=true for the schema. But when I implemented Postgres strategy, I kinda saw it as optional. PG strategy will still count TTL if there is created_at and on updates updated_at, but there is also a fallback for entities that have ttl=true, but no created_at, in which case PG ttl is counted like ngx.time() + ttl. Cassandra never really needed or used created_at for ttl as it just accepts the number as is. With PG there is no native support as of now, so we decided to store expires_at (absolute time) to ttl field, as it makes it easier to cleanup, and exclude expired rows.

@thibaultcha

This comment has been minimized.

Copy link
Member

thibaultcha commented Aug 8, 2018

Had some more concerns but pretty minor at this point, we can practically merge this as-is imho. Left a few notes for you though @bungle

@bungle bungle force-pushed the feat/db-ttl branch from aea798b to 3cc7d19 Aug 9, 2018

if i == argc and is_update and attributes[UNIQUE] then
value = attributes[UNIQUE]
else
ttl_value = escape_literal(connector, ttl_value + time(), fields.ttl)

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 9, 2018

Member

It seems to me like it would be fine to just have this branch? The only purpose the above branches seem to serve is to not repeat the ngx.time() call, and use created_at, or updated_at (which were generated by ngx.time() since they have the auto attribute). That would simplify the implementation quite a bit, unless those branches serve another purpose that I missed, of course.

@bungle bungle merged commit 318c060 into next Aug 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bungle bungle deleted the feat/db-ttl branch Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.