Code review comment for lp://qastaging/~openerp-dev/openobject-server/6.1-multicorn-al

Revision history for this message
Vo Minh Thu (thu) wrote :

Small notes:

- Testing for 'HTTP_X_FORWARDED_HOST' in the environment to activate or not the ProxyFix is probably a security hole.

- You removed the gunicorn.conf.py file, but it is still mentioned in the other configuration file. Probably it would be better to still offer the possibility to run OpenERP behind Gunicorn (and possibly other WSGI servers).

More importantly:

I pretty much dislike the approach and I think we should use another one.

- Inspecting HTTP requests for smarter dispatching to workers can be done using a reverse proxy such as Nginx.

- Having a monitoring process dispatching requests using a pre-fork model to workers is already done is some existing tools. It would make sense to re-use them, preferably using those that are well-known, documented and tested. If we don't find a suitable projects, maybe we can contribute to one almost fullfilling our needs.

It is pretty sure that our own `multicorn` would never be correclty documented and tested. If we ever commit to make our own, it would also be better to make it a separate project, in order to attract more interest (i.e. from outside the OpenERP community, so the project has better chance to stand the test of time).

Existing projects are likely to have interesting features we would not have (such as inc/decrementing worker count, dynamic reconfiguration and hot-code reloading).

- The possibility to run separate processes with separate goals (such as the regular OpenERP server, and the Cron workers) is nice and already available in existing tools.

- Overall separating concerns into different tools/code base/processes is a desirable trait. For instance, mixing command-line arguments for a regular OpenERP server, and for Multicorn-enabled OpenERP is bad, as it is possible to achieve the same result in a cleaner way.

« Back to merge proposal