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
even if it is cached no_ack should affect to the get function #7852
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportBase: 89.77% // Head: 89.77% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #7852 +/- ##
=======================================
Coverage 89.77% 89.77%
=======================================
Files 128 128
Lines 15790 15790
Branches 2111 2111
=======================================
Hits 14176 14176
Misses 1386 1386
Partials 228 228
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -215,7 +215,7 @@ def get(self, timeout=None, propagate=True, interval=0.5, | |||
if on_interval: | |||
_on_interval.then(on_interval) | |||
|
|||
if self._cache: | |||
if self._cache and not no_ack: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please add a unit test to verify this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea about unit test, can you explain how to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To explain a little more in detail, if it becomes a cache through the ready function, no_ack is False and the consumer operates. After that, through the get function, the consumer should operate with no_ack set to True, but the cache value is returned and the function is terminated without operating the consumer with no_ack set to True.
I don't know how make exactly test code. I'm not sure how to write code that correct tests this phenomenon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can check this file to have an idea about related tests https://github.com/celery/celery/blob/master/t/unit/tasks/test_result.py you can try to figure out and learn something new. but if you can not at all then we will see
@Nusnus feel free to give this a tryand let me know what do you think about this change |
From what I can see, the issue makes sense, but I'm not sure if we want this exact solution. @nistick21 In other words, your fix applies well to your case but will cause other people to have a different behavior than expected without an explicit change, which is prone to issues that are hard to find and fix, especially in production systems. So we have to be careful about what we change, even if all of the unit/integration tests of the CI are passing. Lastly, if I assume correctly and the end results are to be able to change the state of the task and get the result without invoking the cache, we need an explicit and clear way of implementing this. An example off the top of my head may be adding a (default def get(self, timeout=None, propagate=True, interval=0.5,
no_ack=True, follow_parents=True, callback=None, on_message=None,
on_interval=None, disable_sync_subtasks=True,
EXCEPTION_STATES=states.EXCEPTION_STATES,
PROPAGATE_STATES=states.PROPAGATE_STATES,
ignore_cache=False):
...
if not ignore_cache and self._cache:
if propagate:
self.maybe_throw(callback=callback)
return self.result Then you can have your example like this: result = add.delay(4, 4)
result.ready()
result.get(ignore_cache=True) result = add.delay(4, 4)
result.get() This will make the code much more readable, as the If this makes sense @nistick21 and you wish to take this further, LMK and I'll try to push you in the right direction. For now here are my two cents :) P.S |
Note: Before submitting this pull request, please review our contributing
guidelines.
Description
I'm beginer of celery. I'm trying to ampq as broker and backend.
in tutorial, result is not removed by get() function
for example,
case1
case2
when I was trying to case 1, not removed from result queue.
but case 2 does.
so I add 1 line in get function.
I'm not good at programming, If you have a another solution. plz tell me.
thanks.