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

Backend doesn't store message failures #156

Closed
davidt99 opened this issue Jan 11, 2019 · 7 comments · Fixed by #293
Closed

Backend doesn't store message failures #156

davidt99 opened this issue Jan 11, 2019 · 7 comments · Fixed by #293

Comments

@davidt99
Copy link
Contributor

@davidt99 davidt99 commented Jan 11, 2019

When working on adding tests for #151, I noticed that the backend doesn't support storing messages failure (failed manually or unhandled exception), causing a timeout when trying to get the result.
I think we need at least store the indication of the message failure. Another approach is to try to raise the exception that was raised in the worker.

@Bogdanp Bogdanp added this to the v2.0.0 milestone Jan 26, 2019
@Bogdanp
Copy link
Owner

@Bogdanp Bogdanp commented Feb 9, 2019

This is intentional and changing it will require an overhaul of how results and result backends work. That's something I'm going to do in 2.0, but that's a ways off for now.

@Bogdanp Bogdanp changed the title Backend doesn't messages failure Backend doesn't store message failures Feb 9, 2019
@gjeusel
Copy link

@gjeusel gjeusel commented Feb 10, 2020

Hello @Bogdanp ! Really nice work the code quality, pleasant to read !

I'm willing to help on this one as it is the only remaining blocking point for dramatiq's adoption in my company.
Would you mind sharing the steps you have in mind to achieve it ?

@Bogdanp
Copy link
Owner

@Bogdanp Bogdanp commented Feb 11, 2020

@gjeusel thanks for offering!

I think the best approach here is for someone to "fork" the result middleware and related code into a separate package and make any breaking changes they need to in order to add that functionality. We can then update the documentation to point to that package as the recommended way to do result management and eventually deprecate and then remove the implementation in Dramatiq in "2.0" down the line and replace it with that package.

This way current users are not going to be affected by the breaking changes and they'll have an upgrade path going forward. New users will have the option to use the result implementation from that package and everybody's happy!

I don't have any suggestions as far as the implementation is concerned. It's been a while since I looked at this.

@takhs91
Copy link
Contributor

@takhs91 takhs91 commented Mar 4, 2020

Hi! Congrats on the good work put into dramatiq.

This issue is also blocker for the company I work for, in order to replace celery with dramatiq as for @gjeusel .

@gjeusel have you started working on that? If yes, I could help you finish it. Otherwise we are interested on working on that.

@Bogdanp why do you think this is a breaking change? We were thinking that the fact that an error happened and possibly the exception message or the whole exception could be stored always.

On the caller's side we can add an new keyword argument to the get_result https://github.com/Bogdanp/dramatiq/blob/master/dramatiq/results/backend.py#L58 like return_failures which can be set to False by default. In that case it could behave as it was.

On the other hand if the flag is set to True it could raise an exception in similar fashion to celery. For the time being it could be a single exception made for that case or in the future it could made specific exceptions based on the returned exception (again similar to celery)

@Bogdanp
Copy link
Owner

@Bogdanp Bogdanp commented Mar 4, 2020

@takhs91 my memory is a bit fuzzy, but I think this would have to be a breaking change because the underlying stores would have to change. I could totally be wrong about this and I'd be happy to help carry through a PR that adds this functionality without breaking existing users' code.

@takhs91
Copy link
Contributor

@takhs91 takhs91 commented Mar 6, 2020

  1. The proposed solution makes sure that no changes will be noticed by the get_result user since the underlying changes are abstracted. Do we also have cases were somebody reads directly from the store? In that case it would be breaking for those since changes will be needed in the result schema
    from result to something like { 'result': 'result', 'success': True} at least. Do you consider this a breaking change?

  2. Otherwise a feature flag could be used in both sides get_results and after_process_message, and wherever else needed that could be set on a global level to make sure the whole feature is turned on only for those that dont' depend on the old results schema inside redis and deprecating the old way later.

  3. To clarify, your proposal was to "fork" the results package inside this repo right ? That would work too but it would require copying many things including the tests and ensure that no further development will be done on the old package to avoid diverging from the new.

So if you think users read directly from redis we should go by option 2 or option 3. I would prefer at least trying option 2 to avoid the aformentioned problems

Otherwise if we only want get_result to be able to behave the same as it was we can go with option 1

I would like to hear your thoughts.

@gjeusel
Copy link

@gjeusel gjeusel commented Mar 11, 2020

Hello @takhs91 !

I'm afraid I havn't started at all...
I switched focus after having found a nasty workaround - a decorator to catch
every exception and forward success or failure -.

Please, don't hesitate to take over this PR !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants