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

Fixing TTL in Cassandra #1125

Merged
merged 2 commits into from
Apr 6, 2016
Merged

Fixing TTL in Cassandra #1125

merged 2 commits into from
Apr 6, 2016

Conversation

subnetmarco
Copy link
Member

Fixes the TTL problem mentioned by @chrisanderton in #1054.

Also introduces the ttl_on_failure property in the configuration file, because 3600 may not be an appropriate value for every environment.

This PR is to be merged in both next and release/0.8.0.

@subnetmarco subnetmarco self-assigned this Apr 4, 2016
@chrisanderton
Copy link

@thefosk this will only fix half of the TTL issue on Postgres - the TTL is now being correctly passed in - and i like that you've refactored it to config rather than seeing a random 7200 in the code :)

But.. when using Postgres the nodes will still be removed from the cluster after a now default 3600 seconds - the TTL will never move because it is calculated based on the created timestamp of the node plus the TTL (created at will never change):

https://github.com/Mashape/kong/blob/fix/ttl/kong/dao/postgres_db.lua#L230

I think there are two options:

  1. Update the TTL based on the expiry of the previous TTL - i.e. existing TTL expiry + 3600 - but this might need an additional query which seems wasteful
  2. Update the TTL using Now + TTL - something like local expire_at = now + (ttl * 1000)

@subnetmarco
Copy link
Member Author

@chrisanderton, the last commit should also cover that use-case.

@subnetmarco subnetmarco added the pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) label Apr 6, 2016
@subnetmarco subnetmarco merged commit b1bf822 into release/0.8.0 Apr 6, 2016
@subnetmarco subnetmarco deleted the fix/ttl branch April 6, 2016 05:57
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 59.231% when pulling 1f8b9c1 on fix/ttl into c08b22a on release/0.8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants