Enable efficient `chord` when using dynamicdb as backend store by dingxiong · Pull Request #8783 · celery/celery

dingxiong · 2024-01-10T01:03:50Z

Description

For canvas::chord, only Redis and Memcached currently use an atomic counter to join the parallel tasks. Other backends use a recurring task to poll the completion of the group every second, which is inefficient.

DynamoDB, on the other hand, supports atomic counter. See ref. So in principle, we can use the same technique as Redis backend store to make chord more efficient in Dynamodb. This PR adds this support for DynamoDB backend.

codecov · 2024-01-10T08:03:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d7700e2) 81.19% compared to head (363e9f5) 81.21%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8783      +/-   ##
==========================================
+ Coverage   81.19%   81.21%   +0.01%     
==========================================
  Files         148      148              
  Lines       18523    18542      +19     
  Branches     3165     3165              
==========================================
+ Hits        15039    15058      +19     
  Misses       3196     3196              
  Partials      288      288

Flag	Coverage Δ
unittests	`81.18% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Nusnus

Nice addition!

Please add automatic tests.
Optionally, also add type hints where it makes sense.

Thank you!

celery/backends/dynamodb.py

dingxiong · 2024-01-10T23:42:32Z

Nice addition!

Please add automatic tests.

Optionally, also add type hints where it makes sense.

Thank you!

Thanks for a quick review. I added some unit tests.
However, I did not find any existing integration test for dynamodb, so I tried to create one for it, but I got stuck on how to replace the backend store only for a few tests. Appreciate your guidance/suggestions.

dingxiong · 2024-01-11T16:21:50Z

Hi @Nusnus @auvipy Could you help take another look?

Nusnus · 2024-01-12T12:32:54Z

Nice addition!

Please add automatic tests.

Optionally, also add type hints where it makes sense.

Thank you!

Thanks for a quick review. I added some unit tests.
However, I did not find any existing integration test for dynamodb, so I tried to create one for it, but I got stuck on how to replace the backend store only for a few tests. Appreciate your guidance/suggestions.

Unfortunately I am less familiar with it too, maybe @auvipy can assist here?

Besides the PR looks good; have you done some manual tests maybe with a real environment?

dingxiong · 2024-01-12T22:30:42Z

Besides the PR looks good; have you done some manual tests maybe with a real environment?

@Nusnus I only tested it with local-dynamodb installation. I will do a test soon with a real AWS account.

Nusnus · 2024-01-12T22:42:25Z

Besides the PR looks good; have you done some manual tests maybe with a real environment?

@Nusnus I only tested it with local-dynamodb installation. I will do a test soon with a real AWS account.

That will very useful!!
Thank you!

P.S
Feel free to paste logs showing everything works ;)

dingxiong · 2024-01-18T23:06:58Z

Hi @Nusnus, Below is the testing result with a real AWS account.

code is below

@celery_app.task
def add(x: int, y: int, n: int = 30):
    from time import sleep
    print(f"starting sleeping {n}")
    sleep(n)
    print(f"finish sleeping {n}")
    return x + y

@celery_app.task
def tsum(nums):
    """Sum an iterable of numbers."""
    return sum(nums)

client side logs are

In [3]: c = chord(header=[add.si(1, 2), add.si(3, 4)], body=tsum.s())
In [4]: result = c()
In [5]: result.get()
Out[5]: 10

server side logs are

Task tasks.slack_tasks.add[16b4a1d8-cd59-4dc7-8d42-af38ac66b9c8] received
Task tasks.slack_tasks.add[16b4a1d8-cd59-4dc7-8d42-af38ac66b9c8] about to execute
Task tasks.slack_tasks.add[a02003a8-48f2-486a-8e32-b224076eff6c] received
Task tasks.slack_tasks.add[a02003a8-48f2-486a-8e32-b224076eff6c] about to execute
starting sleeping 30
finish sleeping 30
Task tasks.slack_tasks.tsum[3e6f1527-7ef9-439e-a017-7c95ff3aa65a] requested
starting sleeping 30
finish sleeping 30
Task tasks.slack_tasks.add[16b4a1d8-cd59-4dc7-8d42-af38ac66b9c8] succeeded in 30.192433045012876s: 3
Task tasks.slack_tasks.tsum[3e6f1527-7ef9-439e-a017-7c95ff3aa65a] received
Task tasks.slack_tasks.tsum[3e6f1527-7ef9-439e-a017-7c95ff3aa65a] about to execute
Task tasks.slack_tasks.add[a02003a8-48f2-486a-8e32-b224076eff6c] succeeded in 30.257312100031413s: 7
Task tasks.slack_tasks.tsum[3e6f1527-7ef9-439e-a017-7c95ff3aa65a] succeeded in 0.03086664993315935s: 10

Also, this is the screenshot of dyanmodb query result showing that chord_count = 0 after the chord is initialized.

auvipy

can you please improve unit test coverage for this backend?

for more information, see https://pre-commit.ci

dingxiong · 2024-01-19T07:19:41Z

can you please improve unit test coverage for this backend?

@auvipy Yes sure. I added one more unit test and codecov score looks good now. But one test matrix Smoke-stamping 3.11 failed. I doubt it is related to my change. Could you help re-run it?

dingxiong · 2024-01-25T09:10:48Z

Hi, @auvipy @Nusnus, any more concerns?

auvipy · 2024-01-29T13:01:09Z

it was all green! welcome aboard!

dingxiong marked this pull request as draft January 10, 2024 01:05

auvipy self-requested a review January 10, 2024 10:05

Nusnus requested changes Jan 10, 2024

View reviewed changes

celery/backends/dynamodb.py Outdated Show resolved Hide resolved

dingxiong force-pushed the xiong-dynamodb-atomic-counter branch 6 times, most recently from 680afb4 to 8851a25 Compare January 10, 2024 19:34

dingxiong marked this pull request as ready for review January 10, 2024 23:38

auvipy reviewed Jan 18, 2024

View reviewed changes

dingxiong force-pushed the xiong-dynamodb-atomic-counter branch from 7d9da79 to 61fd597 Compare January 19, 2024 02:25

dingxiong added 4 commits January 18, 2024 21:25

test

83017cb

add unit test

1f5c311

test

bc0137e

revert bad test chamnge

2e43ace

dingxiong force-pushed the xiong-dynamodb-atomic-counter branch from 4a1785d to 2e43ace Compare January 19, 2024 05:25

[pre-commit.ci] auto fixes from pre-commit.com hooks

363e9f5

for more information, see https://pre-commit.ci

auvipy approved these changes Jan 29, 2024

View reviewed changes

auvipy merged commit 86895a9 into celery:main Jan 29, 2024

auvipy added this to the 5.4 milestone Jan 29, 2024

auvipy added Component: DynamoDB Results Backend PR Type: Enhancement labels Jan 29, 2024

Uh oh!

Conversation

dingxiong commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov bot commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dingxiong commented Jan 10, 2024

Uh oh!

dingxiong commented Jan 11, 2024

Uh oh!

Nusnus commented Jan 12, 2024

Uh oh!

dingxiong commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nusnus commented Jan 12, 2024

Uh oh!

dingxiong commented Jan 18, 2024

Uh oh!

auvipy left a comment

Choose a reason for hiding this comment

Uh oh!

dingxiong commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dingxiong commented Jan 25, 2024

Uh oh!

auvipy commented Jan 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dingxiong commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading

dingxiong commented Jan 12, 2024 •

edited

Loading

dingxiong commented Jan 19, 2024 •

edited

Loading