Asynchronous email delivery should be reconsidered #23

New Issue

reinhard · 2019-02-12T07:47:56Z

reinhard commented

2019-02-12 07:47:56 +00:00

Currently, the forms server sends out emails asynchronously via a celery server with which it communicates through a redis server. This adds a lot of complexity, overhead, and dependencies for a fairly simple task.

If the forms server would just send out the emails directly via mail.fsfe.org, a lot of complexity could be removed from the code, and it would become much easier to maintain it and add new, really useful features.

Currently, the forms server sends out emails asynchronously via a celery server with which it communicates through a redis server. This adds a lot of complexity, overhead, and dependencies for a fairly simple task. If the forms server would just send out the emails directly via mail.fsfe.org, a lot of complexity could be removed from the code, and it would become much easier to maintain it and add new, really useful features.

max.mehl commented

2019-02-12 09:21:22 +00:00

I fully agree. Especially since the Redis licencing of newer versions is highly problematic, we might think of dropping it. About Celery, I have no technical opinion as I haven't ever used it.

Before dropping this however we should also be aware that forms does not only send emails but also store them in JSON files. This could probably also be done sequentially, especially for the comparably low traffic, but it should be considered by a more technically experienced mind than mine ;)

I fully agree. Especially since the Redis licencing of newer versions is highly problematic, we might think of dropping it. About Celery, I have no technical opinion as I haven't ever used it. Before dropping this however we should also be aware that `forms` does not only send emails but also store them in JSON files. This could probably also be done sequentially, especially for the comparably low traffic, but it should be considered by a more technically experienced mind than mine ;)

reinhard added the

prio:high

label 2019-03-27 21:58:52 +00:00

reinhard commented

2019-03-27 21:59:38 +00:00

I'm setting this to prio:high because cleaning and de-bloating the code will make any further improvement much easier to implement.

reinhard commented

2019-05-01 21:37:01 +00:00

I'm getting more and more convinced that we should make this our next priority, since it will make it possible to clean up a lot of unnecessary complexity in the code, and it will make any further improvements much easier.

Looking at the way the web container and the worker container are configured, I think it should not make any difference in which of the two containers the emails are sent and the log file is written.

What could work, AFAICT, is to first replace the asynchronous task calls in common/services/SenderService.py and background/tasks.py with the direct function calls, so everything runs on the "web" container.

If this works, we could go on and:

remove the "worker" container
implement unit tests to help us through the following steps (#28)
step by step remove the now unnecessary abstraction levels
at some point probably remove the need to store anything in Redis
otherwise clean up the code, add some comments, and make it better understandable

@florian.vuillemot what is your opinion about this? Do you think it will work? And do you think it is worth the effort and will help to make the code better?

I'm getting more and more convinced that we should make this our next priority, since it will make it possible to clean up a lot of unnecessary complexity in the code, and it will make any further improvements much easier. Looking at the way the web container and the worker container are configured, I think it should not make any difference in which of the two containers the emails are sent and the log file is written. What could work, AFAICT, is to first replace the asynchronous task calls in common/services/SenderService.py and background/tasks.py with the direct function calls, so everything runs on the "web" container. If this works, we could go on and: * remove the "worker" container * implement unit tests to help us through the following steps (#28) * step by step remove the now unnecessary abstraction levels * at some point probably remove the need to store anything in Redis * otherwise clean up the code, add some comments, and make it better understandable @florian.vuillemot what is your opinion about this? Do you think it will work? And do you think it is worth the effort and will help to make the code better?

florian.vuillemot commented

2019-05-04 13:18:47 +00:00

I'm totally agree with you @reinhard. The code is complex and hard to understand whereas it should be clear and simple !

I will work on this issue next week, when #11 will consider as finish and merged :)

As we will not touch the code base (only the wrapper), do you think that I can start by creating unit tests ?

I'm totally agree with you @reinhard. The code is complex and hard to understand whereas it should be clear and simple ! I will work on this issue next week, when #11 will consider as finish and merged :) As we will not touch the code base (only the wrapper), do you think that I can start by creating unit tests ?

reinhard commented

2019-05-06 09:06:07 +00:00

@florian.vuillemot I agree that it is a good idea to first finish #11 before starting on this.

About the unit tests, I am not 100% sure. As long as the separate web container and worker container exist, test routines for the web container will probably require a running worker container. This might make it more complicated to create and actually run the tests. But if you think it can be easily done, I don't see what should stop you :-)

@florian.vuillemot I agree that it is a good idea to first finish #11 before starting on this. About the unit tests, I am not 100% sure. As long as the separate web container and worker container exist, test routines for the web container will probably require a running worker container. This might make it more complicated to create and actually run the tests. But if you think it can be easily done, I don't see what should stop you :-)

👍 1

florian.vuillemot commented

2019-05-21 21:30:53 +00:00

@reinhard

Next step: 'step by step remove the now unnecessary abstraction levels'

I think that we should work on the configurator.py file. I don't know if we can remove abstraction, but we can be more generic and clean it.

What's your opinion on this?

@reinhard Next step: 'step by step remove the now unnecessary abstraction levels' I think that we should work on the `configurator.py` file. I don't know if we can remove abstraction, but we can be more generic and clean it. What's your opinion on this?

reinhard commented

2019-05-21 22:20:45 +00:00

Code cleanup can certainly be done across all files.

I would be tempted to start with the easy bits, like, for example:

SenderService.validate_and_send_email calls SenderService.deliver_email
SenderService.deliver_email does nothing but call tasks.schedule_email
tasks.schedule_email calls tasks.send_email
tasks.send_email does nothing but call DeliveryService.send
DeliveryService.send finally does send out the email.

Much of this (and similar constructions) could probably be cleaned up without much work, so we don't have 5 layers of function calls, but maybe 2.

However, it really is a matter of taste where to start. I'm okay with whatever you prefer, since it's you who does most of the actual work :-)

Code cleanup can certainly be done across all files. I would be tempted to start with the easy bits, like, for example: * `SenderService.validate_and_send_email` calls `SenderService.deliver_email` * `SenderService.deliver_email` does nothing but call `tasks.schedule_email` * `tasks.schedule_email` calls `tasks.send_email` * `tasks.send_email` does nothing but call `DeliveryService.send` * `DeliveryService.send` finally does send out the email. Much of this (and similar constructions) could probably be cleaned up without much work, so we don't have 5 layers of function calls, but maybe 2. However, it really is a matter of taste where to start. I'm okay with whatever you prefer, since it's you who does most of the actual work :-)

reinhard commented

2019-07-15 12:31:00 +00:00

With #43 I think we can consider this issue fixed.

👍 1

reinhard closed this issue

2019-07-15 12:31:05 +00:00

Sign in to join this conversation.