Add auto expiry for DynamoDB backend by sveniu · Pull Request #5805 · celery/celery

sveniu · 2019-10-30T19:47:20Z

Description

This adds auto-expire support for the DynamoDB backend, via the DynamoDB Time to Live feature.

Highlighting OG DynamoDB backend author @georgepsarakis in case he wants to take a look as well.

This adds auto-expire support for the DynamoDB backend, via the DynamoDB Time to Live feature.

boto3 version 1.9.178 requires botocore>=1.12.178. botocore version 1.12.178 introduces support for the DynamoDB UpdateTimeToLive call. The UpdateTimeToLive call is used by the DynamoDB backend to enable TTL support on a newly created table.

codecov · 2019-10-30T20:45:40Z

Codecov Report

Merging #5805 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5805      +/-   ##
==========================================
+ Coverage   83.13%   83.19%   +0.06%     
==========================================
  Files         144      144              
  Lines       17022    17089      +67     
  Branches     2095     2106      +11     
==========================================
+ Hits        14151    14218      +67     
  Misses       2656     2656              
  Partials      215      215

Impacted Files	Coverage Δ
celery/backends/dynamodb.py	`97.8% <100%> (+1.16%)`	⬆️
celery/app/task.py	`96.68% <0%> (+0.02%)`	⬆️
celery/worker/consumer/events.py	`96.87% <0%> (+0.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8973030...b0c56a8. Read the comment docs.

georgepsarakis

Great work! Thank you very much for your contribution.

I noted a few observations that may be problematic, but overall this seems at a very good state.

Handle TTL enabling/disabling separately from the table get-or-create function. Improve handling of cases where the TTL is already set to the desired state. DynamoDB only allows a single TTL update action within a fairly long time window, so some problematic cases (changing the TTL attribute, enabling/disabling TTL when it was recently modified) will raise exceptions that have to be dealt with.

georgepsarakis · 2019-11-01T17:54:49Z

@sveniu will this work with existing installations? I am quite sure it will, but please let's consider backwards compatibility.

If the boto3 TTL methods are not found, log an informative error. If the user wants to enable TTL, raise an exception; if TTL should be disabled, simply return.

- Handle exceptions by logging the error and re-raising - Log (level debug) when the desired TTL state is already in place

sveniu · 2019-11-01T20:41:30Z

@sveniu will this work with existing installations? I am quite sure it will, but please let's consider backwards compatibility.

I've added some code to gracefully handle older boto3 versions not supporting the TTL methods. This is in addition to the earlier commit that bumps the requirements file to require a boto3 version that actually has TTL support.

As for server-side compatibility: There may be users who use older versions of the downloadable local DynamoDB without support for TTL. I think all AWS regions have TTL support at this moment. In any case, there would be a "normal" exception from boto3 and it would be logged in the exception handler.

georgepsarakis

Thanks for addressing the comments, I added a few more observations for possible enhancements, but overall this should be ready for merge.

Additional changes: - Handle exceptions when calling boto3's describe_time_to_live() - Fix test cases for missing TTL methods

georgepsarakis

Other than the last two comments, I think this should be ready for merge. Nice work @sveniu !

auvipy · 2019-11-09T09:51:03Z

this should be on hold for 4.5

sveniu · 2019-11-11T07:23:55Z

@georgepsarakis With this nearing completion, how does merging work - should I squash (and possibly rewrite the commit message properly) and force-push a single commit, or does squashing happen from your side? I'd like to flesh out the commit message a bit, but no big deal if it's left as it was written in the first commit.

reutsharabani · 2019-11-12T22:33:06Z

+                    "boto3 method '{method}' not found; ensure that "
+                    "boto3>=1.9.178 and botocore>=1.12.178 are installed"
+                ).format(method=method)
+                if not self._has_ttl():


Can't this part stand on its own as a guard? Even at debug level, the log can be a distraction when its not really used.

Also, maybe consider if moving the boto validation code elsewhere is nicer.

And then you could do:

self._validate_boto_version()

It would make the code more direct. This method verification is a necessary nuisance which isn't at the heart of this operation and has enough code to justify its own method.

(it would also be nice to see all missing methods and not just the first, but that is also not a big deal).

Sven Ulland added 2 commits October 30, 2019 20:38

Add auto expiry for DynamoDB backend

c703215

This adds auto-expire support for the DynamoDB backend, via the DynamoDB Time to Live feature.

auvipy added this to the 4.5 milestone Oct 31, 2019

georgepsarakis self-requested a review October 31, 2019 05:04

georgepsarakis self-assigned this Oct 31, 2019

georgepsarakis added the Component: DynamoDB Results Backend label Oct 31, 2019

georgepsarakis suggested changes Oct 31, 2019

View reviewed changes

Comment thread celery/backends/dynamodb.py Outdated

Comment thread celery/backends/dynamodb.py Outdated

Comment thread celery/backends/dynamodb.py

thedrow added the PR Type: Feature label Oct 31, 2019

Sven Ulland added 2 commits November 1, 2019 21:30

Handle older boto3 versions

31c1bfd

If the boto3 TTL methods are not found, log an informative error. If the user wants to enable TTL, raise an exception; if TTL should be disabled, simply return.

Improve logging

b5bf089

- Handle exceptions by logging the error and re-raising - Log (level debug) when the desired TTL state is already in place

georgepsarakis reviewed Nov 2, 2019

View reviewed changes

Comment thread celery/backends/dynamodb.py Outdated

Comment thread celery/backends/dynamodb.py Outdated

Comment thread celery/backends/dynamodb.py Outdated

Comment thread docs/userguide/configuration.rst

Sven Ulland added 2 commits November 4, 2019 11:56

Add and use _has_ttl() convenience method

455d55b

Additional changes: - Handle exceptions when calling boto3's describe_time_to_live() - Fix test cases for missing TTL methods

Update ttl_seconds documentation

9d041e9

georgepsarakis reviewed Nov 9, 2019

View reviewed changes

Comment thread celery/backends/dynamodb.py Outdated

Sven Ulland added 2 commits November 11, 2019 08:21

Log invalid TTL; catch and raise ValueError

224aefd

Separate method _get_table_ttl_description

832bc5b

reutsharabani reviewed Nov 12, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add auto expiry for DynamoDB backend#5805

Add auto expiry for DynamoDB backend#5805
thedrow merged 13 commits into
celery:masterfrom
sveniu:dynamodb-autoexpire

sveniu commented Oct 30, 2019

Uh oh!

codecov Bot commented Oct 30, 2019 •

edited

Loading

Uh oh!

georgepsarakis left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georgepsarakis commented Nov 1, 2019

Uh oh!

sveniu commented Nov 1, 2019

Uh oh!

georgepsarakis left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georgepsarakis left a comment

Uh oh!

Uh oh!

auvipy commented Nov 9, 2019 •

edited

Loading

Uh oh!

sveniu commented Nov 11, 2019

Uh oh!

reutsharabani Nov 12, 2019

Uh oh!

reutsharabani Nov 12, 2019 •

edited

Loading

Uh oh!

sveniu Nov 13, 2019

Uh oh!

Uh oh!

Uh oh!

Conversation

sveniu commented Oct 30, 2019

Description

Uh oh!

codecov Bot commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

georgepsarakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georgepsarakis commented Nov 1, 2019

Uh oh!

sveniu commented Nov 1, 2019

Uh oh!

georgepsarakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georgepsarakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

auvipy commented Nov 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sveniu commented Nov 11, 2019

Uh oh!

reutsharabani Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

reutsharabani Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sveniu Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 30, 2019 •

edited

Loading

auvipy commented Nov 9, 2019 •

edited

Loading

reutsharabani Nov 12, 2019 •

edited

Loading