Skip to content

store children with database backend#8338

Merged
auvipy merged 1 commit intocelery:mainfrom
aaronst:main
Jun 27, 2023
Merged

store children with database backend#8338
auvipy merged 1 commit intocelery:mainfrom
aaronst:main

Conversation

@aaronst
Copy link
Copy Markdown
Contributor

@aaronst aaronst commented Jun 23, 2023

Attempt to fix #8336. The test passes with assert meta['children'] is None, but need to test further with a task that actually has children.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 24, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (11b854a) 87.15% compared to head (a23bd78) 87.15%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #8338   +/-   ##
=======================================
  Coverage   87.15%   87.15%           
=======================================
  Files         148      148           
  Lines       18469    18470    +1     
  Branches     3148     3148           
=======================================
+ Hits        16097    16098    +1     
  Misses       2094     2094           
  Partials      278      278
Flag Coverage Δ
unittests 87.12% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
celery/backends/database/models.py 98.30% <100.00%> (+0.02%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@auvipy auvipy added this to the 5.3.x milestone Jun 24, 2023
auvipy
auvipy reviewed Jun 24, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes looks good to me, would be great if extra test examples are added

@Nusnus
Copy link
Copy Markdown
Member

Nusnus commented Jun 24, 2023 

  =================================== FAILURES ===================================
  _ test_task_redis_result_backend.test_asyncresult_forget_cancels_subscription __
  
  self = <t.integration.test_tasks.test_task_redis_result_backend object at 0x7f7fff656370>
  manager = <celery.contrib.testing.manager.Manager object at 0x7f8001a64910>
  
      def test_asyncresult_forget_cancels_subscription(self, manager):
          channels_before_test = get_active_redis_channels()
      
          result = add.delay(1, 2)
  >       assert set(get_active_redis_channels()) == {
              f"celery-task-meta-{result.id}".encode(), *channels_before_test
          }
  E       AssertionError: assert {b'celery-tas...cba76e5', ...} == {b'celery-tas...cba76e5', ...}
  E         Extra items in the right set:
  E         b'celery-task-meta-b8dc5371-cb87-4711-b950-4ca02f3a4462'
  E         b'celery-task-meta-3936fe15-7b2e-441b-b40c-a31d17ae6666'
  E         b'celery-task-meta-0b80b0ff-9694-4468-b738-4e70659585e8'
  E         Full diff:
  E           {
  Restoring 1 unacknowledged message(s)
  E            b'celery-task-meta-0148c28e-0527-475d-8d20-6f43b7790cf0',...
  E         
  E         ...Full output truncated (189 lines hidden), use '-vv' to show
  
  t/integration/test_tasks.py:489: AssertionError
  ------------------------------ Captured log call -------------------------------
  INFO     celery.worker.strategy:strategy.py:161 Task t.integration.tasks.add[62a654ab-5a84-4a8a-8ff3-aa4f3f0fb7b4] received

I’ve ran the test twice in the CI and this error repeats.

@auvipy
Copy link
Copy Markdown
Member

auvipy commented Jun 24, 2023

i saw this random failure before as well, but in another PR that didn't appear!

@Nusnus
Copy link
Copy Markdown
Member

Nusnus commented Jun 24, 2023

i saw this random failure before as well, but in another PR that didn't appear!

I also see it now in another PR I opened myself.

We need to find out what's going on, regardless, this means this PR is legit :)

@Nusnus Nusnus self-requested a review June 24, 2023 11:08
@Nusnus
Copy link
Copy Markdown
Member

Nusnus commented Jun 24, 2023

@aaronst have you tested this change in a running environment?
Do we know it actually solves the original issue?

@aaronst
Copy link
Copy Markdown
Contributor Author

aaronst commented Jun 27, 2023

@aaronst have you tested this change in a running environment?
Do we know it actually solves the original issue?

Yes tested today and the parent/child relationships are working.

@auvipy auvipy merged commit 51b2846 into celery:main Jun 27, 2023
@Nusnus Nusnus mentioned this pull request Sep 1, 2023
Closed
18 tasks
@auvipy
Copy link
Copy Markdown
Member

auvipy commented Sep 1, 2023

@aaronst the change introduced a possible regression. can you please check it out here #8470

@aaronst
Copy link
Copy Markdown
Contributor Author

aaronst commented Sep 1, 2023

@auvipy looks like a breaking change, not a regression because this is not a bug. The .children field needs to be included, but databases created from previous versions of celery probably don’t have it.

@auvipy
Copy link
Copy Markdown
Member

auvipy commented Sep 2, 2023

I guess we just need schema migrations applied for that

@auvipy auvipy mentioned this pull request Sep 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nusnus Nusnus Nusnus approved these changes

@auvipy auvipy auvipy approved these changes

Assignees

No one assigned

Labels

PR Type: Bugfix

Projects

None yet

Milestone

5.3.x

Development

Successfully merging this pull request may close these issues.

database backend does not store children

3 participants

@aaronst @Nusnus @auvipy