launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/buildmaster/interactor.py'
> --- lib/lp/buildmaster/interactor.py	2015-02-17 05:38:37 +0000
> +++ lib/lp/buildmaster/interactor.py	2016-05-24 14:41:49 +0000
> @@ -46,6 +54,56 @@
>      noisy = False
>  
>  
> +class FileWritingProtocol(Protocol):
> +    """A protocol that saves data to a file."""
> +
> +    def __init__(self, finished, filename):
> +        self.finished = finished
> +        self.filename = filename
> +        self.file = None
> +
> +    def dataReceived(self, data):
> +        if self.file is None:
> +            self.file = open(self.filename, "wb")
> +        try:
> +            self.file.write(data)
> +        except IOError:
> +            try:
> +                self.file.close()
> +            except IOError:
> +                pass
> +            self.file = None
> +            self.finished.errback()

Should we turn the IOError into a Failure here so the errback gets more than nothing?

> +
> +    def connectionLost(self, reason):
> +        try:
> +            self.file.close()
> +        except IOError:
> +            self.finished.errback()
> +        else:
> +            if reason.check(ResponseDone):
> +                self.finished.callback(None)
> +            else:
> +                self.finished.errback(reason)
> +
> +
> +_default_pool = None
> +
> +
> +def default_pool(reactor=None):
> +    global _default_pool
> +    if reactor is None:
> +        reactor = default_reactor
> +    if _default_pool is None:
> +        # Circular import.
> +        from lp.buildmaster.manager import SlaveScanner
> +        # Short cached connection timeout to avoid potential weirdness with
> +        # virtual builders that reboot frequently.
> +        _default_pool = HTTPConnectionPool(reactor)
> +        _default_pool.cachedConnectionTimeout = SlaveScanner.SCAN_INTERVAL

As discussed earlier, we probably want to increase maxPersistentPerHost from its default of 2. I'm not sure how well launchpad-buildd likes pipelining, and we have the LFP issue to contend with (not just slow-start -- window issues mean that even very long connections are slow).

> +    return _default_pool
> +
> +
>  class BuilderSlave(object):
>      """Add in a few useful methods for the XMLRPC slave.
>  


-- 
https://code.launchpad.net/~cjwatson/launchpad/buildmaster-twisted-agent/+merge/295593
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.