Merge lp://staging/~openerp-dev/openobject-server/6.1-multicorn-al into lp://staging/openobject-server/6.1

Proposed by Antony Lesuisse (OpenERP)
Status: Needs review
Proposed branch: lp://staging/~openerp-dev/openobject-server/6.1-multicorn-al
Merge into: lp://staging/openobject-server/6.1
Diff against target: 1101 lines (+486/-397)
10 files modified
gunicorn.conf.py (+0/-62)
openerp-cron-worker (+0/-111)
openerp-server (+10/-16)
openerp-wsgi.py (+53/-0)
openerp/__init__.py (+1/-1)
openerp/addons/base/ir/ir_cron.py (+0/-56)
openerp/service/__init__.py (+30/-9)
openerp/tools/config.py (+25/-22)
openerp/wsgi/core.py (+367/-86)
openerp/wsgi/proxied.py (+0/-34)
To merge this branch: bzr merge lp://staging/~openerp-dev/openobject-server/6.1-multicorn-al
Reviewer Review Type Date Requested Status
Christophe Simonis (OpenERP) Needs Resubmitting
OpenERP Core Team Pending
Review via email: mp+109544@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Christophe Simonis (OpenERP) (kangol) wrote :

Not for 6.1

review: Needs Resubmitting
Revision history for this message
Raphael Collet (OpenERP) (rco-openerp) wrote :

Is --multiprocess a standard option name? I dislike it, because its name suggests a *boolean* option. Make uses --jobs (shortcut -j) for the same purpose, and I think that a bunch of tools use --jobs. I also saw --processes, that I find nice and unambiguous.

Some technical remarks:
- Remove line 375 in diff: apparently openerp.multi_process is not used anywhere else.
- Lines 625, 694 in diff: use "True" instead of "1".
- The class names "Unicron" and "Unicorn" are only a typo away. This confused me the first time I saw it. I suggest to increase their typo distance, and use something like "UnicornCron" or "UniCRON" to reduce confusion.

Raphael

Revision history for this message
Christophe Simonis (OpenERP) (kangol) wrote :

Is psutil is really required ?

Revision history for this message
Antony Lesuisse (OpenERP) (al-openerp) wrote :

> Is --multiprocess a standard option name? I dislike it, because its name
> suggests a *boolean* option. Make uses --jobs (shortcut -j) for the same
> purpose, and I think that a bunch of tools use --jobs. I also saw
> --processes, that I find nice and unambiguous.

--workers ok ?

> Some technical remarks:
> - Remove line 375 in diff: apparently openerp.multi_process is not used
> anywhere else.

it is used, it enables db signaling for registry reload

> - Lines 625, 694 in diff: use "True" instead of "1".
> - The class names "Unicron" and "Unicorn" are only a typo away. This confused
> me the first time I saw it. I suggest to increase their typo distance, and
> use something like "UnicornCron" or "UniCRON" to reduce confusion.

It's was pun, i plan to rename them to HTTPWorker and CronWorker before the merge. :)

4215. By Antony Lesuisse (OpenERP)

rename option to workers, application and loading cleanups

4216. By Antony Lesuisse (OpenERP)

remove proxymode in favour of auto detection

4217. By Antony Lesuisse (OpenERP)

wsgi entry point for uwsgi mod_wsgi and gunicorn

4218. By Antony Lesuisse (OpenERP)

clean shutdown

4219. By Antony Lesuisse (OpenERP)

cron start_internal

4220. By Antony Lesuisse (OpenERP)

[DOC] high_availability

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.

Unmerged revisions

4220. By Antony Lesuisse (OpenERP)

[DOC] high_availability

4219. By Antony Lesuisse (OpenERP)

cron start_internal

4218. By Antony Lesuisse (OpenERP)

clean shutdown

4217. By Antony Lesuisse (OpenERP)

wsgi entry point for uwsgi mod_wsgi and gunicorn

4216. By Antony Lesuisse (OpenERP)

remove proxymode in favour of auto detection

4215. By Antony Lesuisse (OpenERP)

rename option to workers, application and loading cleanups

4214. By Antony Lesuisse (OpenERP)

start_multi cleanup

4213. By Antony Lesuisse (OpenERP)

unicron process cron every 60s

4212. By Antony Lesuisse (OpenERP)

unicron process cron, remove awful ir_cron._run method

4211. By Antony Lesuisse (OpenERP)

unicron list databases cleanly

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.