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

Deprecate gaiohttp worker and document alternatives #1569

Merged
merged 2 commits into from
Oct 31, 2017
Merged

Deprecate gaiohttp worker and document alternatives #1569

merged 2 commits into from
Oct 31, 2017

Conversation

berkerpeksag
Copy link
Collaborator

Fixes #1338

Copy link
Collaborator

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for PR!

@berkerpeksag
Copy link
Collaborator Author

Thanks for the review, @asvetlov. I've just tweaked the docs a bit. @tilgovi can I get a review from you too?

@@ -123,13 +123,20 @@ A string referring to one of the following bundled classes:
* ``gevent`` - Requires gevent >= 0.13
* ``tornado`` - Requires tornado >= 0.2
* ``gthread`` - Python 2 requires the futures package to be installed
* ``gaiohttp`` - Requires Python 3.4 and aiohttp >= 0.21.5
* ``gaiohttp`` - Deprecated. Please use
Copy link
Owner

@benoitc benoitc Aug 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should just mark it as deprecated. documentation is already here to point to external asyncio workers. "The 'gaiohttp' worker is deprecated. Please look at "link to worker page" for an alternative worker supporting asyncio."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

man, i just started using it this week! :)


Optionally, you can provide your own worker by giving Gunicorn a
Python path to a subclass of ``gunicorn.workers.base.Worker``.
This alternative syntax will load the gevent class:
``gunicorn.workers.ggevent.GeventWorker``.

.. deprecated:: 19.8
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -624,12 +624,19 @@ class WorkerClass(Setting):
* ``gevent`` - Requires gevent >= 0.13
* ``tornado`` - Requires tornado >= 0.2
* ``gthread`` - Python 2 requires the futures package to be installed
* ``gaiohttp`` - Requires Python 3.4 and aiohttp >= 0.21.5
* ``gaiohttp`` - Deprecated. Please use
``aiohttp.worker.GunicornWebWorker`` instead. See :ref:`asyncio-workers`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem. just tell it's deprecated.

from gunicorn.workers._gaiohttp import AiohttpWorker

util.warn(
"The 'gaiohttp' worker is deprecated. Please install 'aiohttp' "
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would would reformulate by saying that "The 'gaiohttp' worker is deprecated. Please look at "link to worker page" for an alternative worker supporting asyncio.

@benoitc
Copy link
Owner

benoitc commented Aug 22, 2017

cc @berkerpeksag

Copy link
Collaborator

@tilgovi tilgovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks for doing this. Agree with Benoit that the text doesn't need to suggest aiohttp.worker.GunicornWebWorker in particular, just a link to the docs that discuss the three options.

@kennethreitz
Copy link
Collaborator

Excited to get this merged.

except ImportError:
from gunicorn.workers._gaiohttp import AiohttpWorker

util.warn(
Copy link
Contributor

@Code0x58 Code0x58 Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this kind of thing should be using Python's inbuilt warning logging facilities, with a DeprecationWarning. This may be a separarte issue if there are other similar depreciation warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made #1590/#1591 to use warnings in the other two places.

@tilgovi
Copy link
Collaborator

tilgovi commented Sep 16, 2017

Anything else to do here?

@berkerpeksag
Copy link
Collaborator Author

Anything else to do here?

Sorry for my late response. I'm going to address review comments and merge it this weekend.

@berkerpeksag
Copy link
Collaborator Author

Agree with Benoit that the text doesn't need to suggest aiohttp.worker.GunicornWebWorker in particular, just a link to the docs that discuss the three options.

@tilgovi could you be more specific and give a link of that page? Did you mean http://docs.gunicorn.org/en/latest/design.html#async-workers ? I think it would be nice to point out a drop-in replacement in --worker-class documentation.

Here's my diff:

diff --git a/docs/source/run.rst b/docs/source/run.rst
index 516d2ea..4eacb0a 100644
--- a/docs/source/run.rst
+++ b/docs/source/run.rst
@@ -61,7 +61,7 @@ Commonly Used Arguments
   to run. You'll definitely want to read the production page for the
   implications of this parameter. You can set this to ``$(NAME)``
   where ``$(NAME)`` is one of ``sync``, ``eventlet``, ``gevent``,
-  ``tornado``, ``gthread``, ``gaiohttp`` (deprecated in Gunicorn 19.8).
+  ``tornado``, ``gthread``, ``gaiohttp`` (deprecated).
   ``sync`` is the default. See the :ref:`worker-class` documentation for more
   information.
 * ``-n APP_NAME, --name=APP_NAME`` - If setproctitle_ is installed you can
diff --git a/gunicorn/config.py b/gunicorn/config.py
index b74227d..169f10f 100644
--- a/gunicorn/config.py
+++ b/gunicorn/config.py
@@ -624,9 +624,7 @@ class WorkerClass(Setting):
         * ``gevent``   - Requires gevent >= 0.13
         * ``tornado``  - Requires tornado >= 0.2
         * ``gthread``  - Python 2 requires the futures package to be installed
-        * ``gaiohttp`` - Deprecated. Please use
-          ``aiohttp.worker.GunicornWebWorker`` instead. See :ref:`asyncio-workers`
-          for more information on how to use it.
+        * ``gaiohttp`` - Deprecated.
 
         Optionally, you can provide your own worker by giving Gunicorn a
         Python path to a subclass of ``gunicorn.workers.base.Worker``.
diff --git a/gunicorn/workers/gaiohttp.py b/gunicorn/workers/gaiohttp.py
index a5412cc..7d0227a 100644
--- a/gunicorn/workers/gaiohttp.py
+++ b/gunicorn/workers/gaiohttp.py
@@ -19,8 +19,8 @@ if sys.version_info >= (3, 4):
             from gunicorn.workers._gaiohttp import AiohttpWorker
 
         util.warn(
-            "The 'gaiohttp' worker is deprecated. Please install 'aiohttp' "
-            "and pass 'aiohttp.worker.GunicornWebWorker' to --worker-class."
+            "The 'gaiohttp' worker is deprecated. See --worker-class "
+            "documentation for more information."
         )
         __all__ = ['AiohttpWorker']
 else:

@tilgovi
Copy link
Collaborator

tilgovi commented Oct 30, 2017

I think linking to the new docs on AsyncIO from the documentation / usage for --worker-class is great, but I don't mind a direct reference to aiohttp.worker.GunicornWorker in the usage either. Both are fine!

@berkerpeksag berkerpeksag merged commit 2dd7321 into benoitc:master Oct 31, 2017
@berkerpeksag berkerpeksag deleted the gaiohttp branch October 31, 2017 04:30
mjjbell pushed a commit to mjjbell/gunicorn that referenced this pull request Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants