launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Information



Diff comments:

> diff --git a/lib/lp/scripts/runlaunchpad.py b/lib/lp/scripts/runlaunchpad.py
> index 0557ee8..79ba3d1 100644
> --- a/lib/lp/scripts/runlaunchpad.py
> +++ b/lib/lp/scripts/runlaunchpad.py
> @@ -332,7 +334,31 @@ def start_testapp(argv=list(sys.argv)):
>                  pass
>  
>  
> +def gunicornify_zope_config_file():
> +    """Creates a new launchpad.config file removing directives related to
> +    Zope Server that shouldn't be used when running on gunicorn.
> +    """
> +    original_filename = config.zope_config_file
> +    with open(original_filename) as fd:
> +        content = fd.read()
> +
> +    # Remove unwanted tags.
> +    for tag in ['server', 'accesslog']:
> +        content = re.sub(
> +            r"<%s>(.*)</%s>" % (tag, tag), "", content, flags=re.S)
> +
> +    # Remove unwanted single-line directives.
> +    for directive in ['interrupt-check-interval']:
> +        content = re.sub(r"%s .*" % directive, "", content)
> +
> +    new_filename = tempfile.mktemp(prefix='launchpad.conf-gunicorn-')
> +    with open(new_filename, 'w') as fd:
> +        fd.write(content)
> +    config.zope_config_file = new_filename

This seems likely to leave a fair bit of temporary file noise lying around the filesystem, so it would be good to come up with a way to avoid this.

zope.app.wsgi.config supports its configfile parameter being either an opened file or a path.  LaunchpadConfig._getZConfig could easily be modified to do the same.  Would a viable option be to adjust LaunchpadConfig._getZConfig and then set config.zope_config_file to a BytesIO, thus keeping this all in memory?

> +
> +
>  def gunicorn_main():
> +    gunicornify_zope_config_file()
>      orig_argv = sys.argv
>      try:
>          sys.argv = [
> diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py
> index 7325d9f..b245c92 100644
> --- a/lib/lp/services/webapp/publication.py
> +++ b/lib/lp/services/webapp/publication.py
> @@ -7,6 +7,7 @@ __all__ = [
>      'LaunchpadBrowserPublication',
>      ]
>  
> +import logging

This seems to be unused.

>  import re
>  import sys
>  import threading
> @@ -871,6 +872,8 @@ def tracelog(request, prefix, msg):
>      easier. ``prefix`` should be unique and contain no spaces, and
>      preferably a single character to save space.
>      """
> -    tracelog = ITraceLog(request, None)
> -    if tracelog is not None:
> -        tracelog.log('%s %s' % (prefix, six.ensure_str(msg, 'US-ASCII')))
> +    if not config.use_gunicorn:
> +        msg = '%s %s' % (prefix, six.ensure_str(msg, 'US-ASCII'))
> +        tracelog = ITraceLog(request, None)
> +        if tracelog is not None:
> +            tracelog.log(msg)

The tracelog can be actively useful sometimes to get fine-grained information about the processing of a particular request, which I'm not sure is quite superseded by gunicorn.  Is there a reason it couldn't be retained?

> diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
> index 9089c60..98c8149 100644
> --- a/lib/lp/testing/layers.py
> +++ b/lib/lp/testing/layers.py
> @@ -155,7 +160,6 @@ from lp.testing import (
>      reset_logging,
>      )
>  from lp.testing.pgsql import PgTestSetup
> -import zcml

The zcml module is local to the Launchpad tree, so should stay in the third import block rather than the second.  It should probably be added to LOCAL_PACKAGES in utilities/format-imports.

>  
>  
>  COMMA = ','
> @@ -1794,6 +1798,14 @@ class LayerProcessController:
>              raise LayerIsolationError(
>                  "App server died in this test (status=%s):\n%s" % (
>                      cls.appserver.returncode, cls.appserver.stdout.read()))
> +        # Cleanup the app server's output buffer between tests.
> +        while True:
> +            # Read while we have something available at the stdout.
> +            r, w, e = select.select([cls.appserver.stdout], [], [], 0)
> +            if cls.appserver.stdout in r:
> +                cls.appserver.stdout.readline()

Possibly .read() rather than .readline()?  I'm not sure we care much about line breaks here.

> +            else:
> +                break
>          DatabaseLayer.force_dirty_database()
>  
>      @classmethod


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/396691
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:gunicorn-appserver-tests.