Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad
Diff comments:
>
> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py 2018-06-14 17:09:31 +
> +++ lib/lp/code/model/branch.py 2018-06-14 17:09:31 +
> @@ -783,6 +789,63 @@
> RevisionAuthor, revisions, ['revision_author_id'])
> return DecoratedResultSet(result, pre_iter_hook=eager_load)
>
> +def getBlob(self, filename, revision_id=None, enable_memcache=None,
> +logger=None):
> +"""See `IBranch`."""
> +hosting_client = getUtility(IBranchHostingClient)
> +if enable_memcache is None:
> +enable_memcache = not getFeatureFlag(
> +u'code.bzr.blob.disable_memcache')
> +if revision_id is None:
> +revision_id = self.last_scanned_id
> +if revision_id is None:
> +# revision_id may still be None if the branch scanner hasn't
> +# scanned this branch yet. In this case, we won't be able to
> +# guarantee that subsequent calls to this method within the same
> +# transaction with revision_id=None will see the same revision,
> +# and we won't be able to cache file lists. Neither is fatal,
> +# and this should be relatively rare.
> +enable_memcache = False
> +dirname = os.path.dirname(filename)
> +unset = object()
> +file_list = unset
It's not unused; getBlob stores None in memcached in the event that the file
has been positively confirmed not to exist in that revision.
> +if enable_memcache:
> +memcache_client = getUtility(IMemcacheClient)
> +instance_name = urlsplit(
> +config.codehosting.internal_bzr_api_endpoint).hostname
> +memcache_key = '%s:bzr-file-list:%s:%s:%s' % (
> +instance_name, self.id, revision_id, dirname)
> +if not isinstance(memcache_key, bytes):
> +memcache_key = memcache_key.encode('UTF-8')
> +cached_file_list = memcache_client.get(memcache_key)
> +if cached_file_list is not None:
> +try:
> +file_list = json.loads(cached_file_list)
> +except Exception:
> +logger.exception(
> +'Cannot load cached file list for %s:%s:%s;
> deleting' %
> +(self.unique_name, revision_id, dirname))
> +memcache_client.delete(memcache_key)
> +if file_list is unset:
> +try:
> +inventory = hosting_client.getInventory(
> +self.unique_name, dirname, rev=revision_id)
> +file_list = {
> +entry['filename']: entry['file_id']
> +for entry in inventory['filelist']}
> +except BranchFileNotFound:
> +file_list = None
> +if enable_memcache:
> +# Cache the file list in case there's a request for another
> +# file in the same directory.
> +memcache_client.set(memcache_key, json.dumps(file_list))
> +file_id = (file_list or {}).get(os.path.basename(filename))
> +if file_id is None:
> +raise BranchFileNotFound(
> +self.unique_name, filename=filename, rev=revision_id)
> +return hosting_client.getBlob(
> +self.unique_name, file_id, rev=revision_id)
> +
> def getDiff(self, new, old=None):
> """See `IBranch`."""
> hosting_client = getUtility(IBranchHostingClient)
>
> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py 2018-04-21 10:01:22 +
> +++ lib/lp/snappy/browser/snap.py 2018-06-14 17:09:31 +
> @@ -424,34 +422,19 @@
> @property
> def initial_values(self):
> store_name = None
> -if self.has_snappy_distro_series and
> IGitRef.providedBy(self.context):
> +if self.has_snappy_distro_series:
> # Try to extract Snap store name from snapcraft.yaml file.
> try:
> -paths = (
> -'snap/snapcraft.yaml',
> -'snapcraft.yaml',
> -'.snapcraft.yaml',
> -)
> -for i, path in enumerate(paths):
> -try:
> -blob = self.context.repository.getBlob(
> -path, self.context.name)
> -break
> -except GitRepositoryBlobNotFound:
> -if i == len(paths) - 1:
> -raise
> -# Beware of unsafe yaml.load()!
> -store_name = yaml.safe_load(blob).get('name')
> -except GitRepositoryScanFault:
> -log.exception("Failed to get Snap manifest from Git %s",
> - self.context.unique_name)
> -
[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad
The proposal to merge lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~cjwatson/launchpad/snap-initial-name-bzr/+merge/345757 -- Your team Launchpad code reviewers is subscribed to branch lp:launchpad. ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
Re: [Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad
Review: Approve code
Diff comments:
>
> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py 2018-06-14 17:09:31 +
> +++ lib/lp/code/model/branch.py 2018-06-14 17:09:31 +
> @@ -783,6 +789,63 @@
> RevisionAuthor, revisions, ['revision_author_id'])
> return DecoratedResultSet(result, pre_iter_hook=eager_load)
>
> +def getBlob(self, filename, revision_id=None, enable_memcache=None,
> +logger=None):
> +"""See `IBranch`."""
> +hosting_client = getUtility(IBranchHostingClient)
> +if enable_memcache is None:
> +enable_memcache = not getFeatureFlag(
> +u'code.bzr.blob.disable_memcache')
> +if revision_id is None:
> +revision_id = self.last_scanned_id
> +if revision_id is None:
> +# revision_id may still be None if the branch scanner hasn't
> +# scanned this branch yet. In this case, we won't be able to
> +# guarantee that subsequent calls to this method within the same
> +# transaction with revision_id=None will see the same revision,
> +# and we won't be able to cache file lists. Neither is fatal,
> +# and this should be relatively rare.
> +enable_memcache = False
> +dirname = os.path.dirname(filename)
> +unset = object()
> +file_list = unset
Can you just use None here, since it's otherwise unused?
> +if enable_memcache:
> +memcache_client = getUtility(IMemcacheClient)
> +instance_name = urlsplit(
> +config.codehosting.internal_bzr_api_endpoint).hostname
> +memcache_key = '%s:bzr-file-list:%s:%s:%s' % (
> +instance_name, self.id, revision_id, dirname)
> +if not isinstance(memcache_key, bytes):
> +memcache_key = memcache_key.encode('UTF-8')
> +cached_file_list = memcache_client.get(memcache_key)
> +if cached_file_list is not None:
> +try:
> +file_list = json.loads(cached_file_list)
> +except Exception:
> +logger.exception(
> +'Cannot load cached file list for %s:%s:%s;
> deleting' %
> +(self.unique_name, revision_id, dirname))
> +memcache_client.delete(memcache_key)
> +if file_list is unset:
> +try:
> +inventory = hosting_client.getInventory(
> +self.unique_name, dirname, rev=revision_id)
> +file_list = {
> +entry['filename']: entry['file_id']
> +for entry in inventory['filelist']}
> +except BranchFileNotFound:
> +file_list = None
> +if enable_memcache:
> +# Cache the file list in case there's a request for another
> +# file in the same directory.
> +memcache_client.set(memcache_key, json.dumps(file_list))
> +file_id = (file_list or {}).get(os.path.basename(filename))
> +if file_id is None:
> +raise BranchFileNotFound(
> +self.unique_name, filename=filename, rev=revision_id)
> +return hosting_client.getBlob(
> +self.unique_name, file_id, rev=revision_id)
> +
> def getDiff(self, new, old=None):
> """See `IBranch`."""
> hosting_client = getUtility(IBranchHostingClient)
>
> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py 2018-04-21 10:01:22 +
> +++ lib/lp/snappy/browser/snap.py 2018-06-14 17:09:31 +
> @@ -424,34 +422,19 @@
> @property
> def initial_values(self):
> store_name = None
> -if self.has_snappy_distro_series and
> IGitRef.providedBy(self.context):
> +if self.has_snappy_distro_series:
> # Try to extract Snap store name from snapcraft.yaml file.
> try:
> -paths = (
> -'snap/snapcraft.yaml',
> -'snapcraft.yaml',
> -'.snapcraft.yaml',
> -)
> -for i, path in enumerate(paths):
> -try:
> -blob = self.context.repository.getBlob(
> -path, self.context.name)
> -break
> -except GitRepositoryBlobNotFound:
> -if i == len(paths) - 1:
> -raise
> -# Beware of unsafe yaml.load()!
> -store_name = yaml.safe_load(blob).get('name')
> -except GitRepositoryScanFault:
> -log.exception("Failed to get Snap manifest from Git %s",
> - self.context.unique_name)
> -except (AttributeError, yaml.YAMLError):
> -
[Launchpad-reviewers] [Merge] lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-initial-name-bzr
into lp:launchpad with lp:~cjwatson/launchpad/better-bzr-mp-diffs as a
prerequisite.
Commit message:
Extract initial Snap.store_name from snapcraft.yaml for Bazaar as well as Git.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-initial-name-bzr/+merge/345757
The potential timeout issues will be at least as bad as with git, but
BranchHostingClient has all the same sort of timeout management code as we put
together for GitHostingClient, which should mitigate that.
--
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2018-05-17 14:25:53 +
+++ lib/lp/code/interfaces/branch.py 2018-05-17 14:25:53 +
@@ -765,6 +765,15 @@
:param launchbag: `ILaunchBag`.
"""
+def getBlob(filename, revision_id=None):
+"""Get a blob by file name from this branch.
+
+:param filename: Relative path of a file in the branch.
+:param revision_id: An optional revision ID. Defaults to the last
+scanned revision ID of the branch.
+:return: The blob content as a byte string.
+"""
+
def getDiff(new, old):
"""Get the diff between two revisions in this branch.
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2018-05-17 14:25:53 +
+++ lib/lp/code/model/branch.py 2018-05-17 14:25:53 +
@@ -10,12 +10,15 @@
from datetime import datetime
from functools import partial
+import json
import operator
+import os.path
from bzrlib import urlutils
from bzrlib.revision import NULL_REVISION
from lazr.lifecycle.event import ObjectCreatedEvent
import pytz
+from six.moves.urllib_parse import urlsplit
from sqlobject import (
ForeignKey,
IntCol,
@@ -84,6 +87,7 @@
)
from lp.code.errors import (
AlreadyLatestFormat,
+BranchFileNotFound,
BranchMergeProposalExists,
BranchTargetError,
BranchTypeError,
@@ -172,10 +176,12 @@
ArrayAgg,
ArrayIntersects,
)
+from lp.services.features import getFeatureFlag
from lp.services.helpers import shortlist
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
from lp.services.mail.notificationrecipientset import NotificationRecipientSet
+from lp.services.memcache.interfaces import IMemcacheClient
from lp.services.propertycache import (
cachedproperty,
get_property_cache,
@@ -783,6 +789,63 @@
RevisionAuthor, revisions, ['revision_author_id'])
return DecoratedResultSet(result, pre_iter_hook=eager_load)
+def getBlob(self, filename, revision_id=None, enable_memcache=None,
+logger=None):
+"""See `IBranch`."""
+hosting_client = getUtility(IBranchHostingClient)
+if enable_memcache is None:
+enable_memcache = not getFeatureFlag(
+u'code.bzr.blob.disable_memcache')
+if revision_id is None:
+revision_id = self.last_scanned_id
+if revision_id is None:
+# revision_id may still be None if the branch scanner hasn't
+# scanned this branch yet. In this case, we won't be able to
+# guarantee that subsequent calls to this method within the same
+# transaction with revision_id=None will see the same revision,
+# and we won't be able to cache file lists. Neither is fatal,
+# and this should be relatively rare.
+enable_memcache = False
+dirname = os.path.dirname(filename)
+unset = object()
+file_list = unset
+if enable_memcache:
+memcache_client = getUtility(IMemcacheClient)
+instance_name = urlsplit(
+config.codehosting.internal_bzr_api_endpoint).hostname
+memcache_key = '%s:bzr-file-list:%s:%s:%s' % (
+instance_name, self.id, revision_id, dirname)
+if not isinstance(memcache_key, bytes):
+memcache_key = memcache_key.encode('UTF-8')
+cached_file_list = memcache_client.get(memcache_key)
+if cached_file_list is not None:
+try:
+file_list = json.loads(cached_file_list)
+except Exception:
+logger.exception(
+'Cannot load cached file list for %s:%s:%s; deleting' %
+(self.unique_name, revision_id, dirname))
+memcache_client.delete(memcache_key)
+if file_list is unset:
+try:
+inventory = hosting_client.getInventory(
+self.unique_name, dirname, rev=revision_id)
+file_list = {
+
