Patchwork [3,of,3] pull: only prefetch bookmark when using bundle1

login
register
mail settings
Submitter Pierre-Yves David
Date May 29, 2015, 10:25 p.m.
Message ID <a9acfde2c3126309056a.1432938315@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/9400/
State Accepted
Headers show

Comments

Pierre-Yves David - May 29, 2015, 10:25 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1432727823 25200
#      Wed May 27 04:57:03 2015 -0700
# Node ID a9acfde2c3126309056a92d421913599ff3781fe
# Parent  f9a958f8bc37c17ff8b10eee45ee51379ab90124
pull: only prefetch bookmark when using bundle1

All bundle2 servers now support the 'listkeys' part(1), so we'll alway be able
to fetch bookmarks data at the same time than the changeset. This should be
enough to avoid the one race condition that this bookmark prefetching is trying
to work around. It even allow future server to make sure everything is generated
from the same "transaction" if they become capable of such. The current code was
already overwriting the prefetched value with the one in bundle2 anyway.  Note
that this is not preventing all races conditions in related to bookmark in 'hg
pull' it makes nothing better and nothing worse.

Reducing the number of listkeys calls will reduce the latency on pull.

The pre-fetch is also moved into a discovery step because it seems to belong
there.

(1) Because all servers not speaking 'pushkey' parts are conmpatible with the
'HG2X' protocol only.
Augie Fackler - June 1, 2015, 3:39 a.m.
On May 29, 2015, at 6:25 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1432727823 25200
> #      Wed May 27 04:57:03 2015 -0700
> # Node ID a9acfde2c3126309056a92d421913599ff3781fe
> # Parent  f9a958f8bc37c17ff8b10eee45ee51379ab90124
> pull: only prefetch bookmark when using bundle1

These were part of my bulk review of marmoute’s series and are now queued. Thanks!

> 
> All bundle2 servers now support the 'listkeys' part(1), so we'll alway be able
> to fetch bookmarks data at the same time than the changeset. This should be
> enough to avoid the one race condition that this bookmark prefetching is trying
> to work around. It even allow future server to make sure everything is generated
> from the same "transaction" if they become capable of such. The current code was
> already overwriting the prefetched value with the one in bundle2 anyway.  Note
> that this is not preventing all races conditions in related to bookmark in 'hg
> pull' it makes nothing better and nothing worse.
> 
> Reducing the number of listkeys calls will reduce the latency on pull.
> 
> The pre-fetch is also moved into a discovery step because it seems to belong
> there.
> 
> (1) Because all servers not speaking 'pushkey' parts are conmpatible with the
> 'HG2X' protocol only.
> 
> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -893,11 +893,10 @@ def pull(repo, remote, heads=None, force
>             msg = _("required features are not"
>                     " supported in the destination:"
>                     " %s") % (', '.join(sorted(missing)))
>             raise util.Abort(msg)
> 
> -    pullop.remotebookmarks = remote.listkeys('bookmarks')
>     lock = pullop.repo.lock()
>     try:
>         pullop.trmanager = transactionmanager(repo, 'pull', remote.url())
>         _pulldiscovery(pullop)
>         if _canusebundle2(pullop):
> @@ -941,10 +940,20 @@ def _pulldiscovery(pullop):
>     """Run all discovery steps"""
>     for stepname in pulldiscoveryorder:
>         step = pulldiscoverymapping[stepname]
>         step(pullop)
> 
> +@pulldiscovery('b1:bookmarks')
> +def _pullbookmarkbundle1(pullop):
> +    """fetch bookmark data in bundle1 case
> +
> +    If not using bundle2, we have to fetch bookmark before doing changesets
> +    discovery to reduce the change and impact of race condition."""
> +    if not _canusebundle2(pullop): # all bundle2 server now support listkeys
> +        pullop.remotebookmarks = pullop.remote.listkeys('bookmarks')
> +
> +
> @pulldiscovery('changegroup')
> def _pulldiscoverychangegroup(pullop):
>     """discovery phase for the pull
> 
>     Current handle changeset discovery only, will change handle all discovery
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -893,11 +893,10 @@  def pull(repo, remote, heads=None, force
             msg = _("required features are not"
                     " supported in the destination:"
                     " %s") % (', '.join(sorted(missing)))
             raise util.Abort(msg)
 
-    pullop.remotebookmarks = remote.listkeys('bookmarks')
     lock = pullop.repo.lock()
     try:
         pullop.trmanager = transactionmanager(repo, 'pull', remote.url())
         _pulldiscovery(pullop)
         if _canusebundle2(pullop):
@@ -941,10 +940,20 @@  def _pulldiscovery(pullop):
     """Run all discovery steps"""
     for stepname in pulldiscoveryorder:
         step = pulldiscoverymapping[stepname]
         step(pullop)
 
+@pulldiscovery('b1:bookmarks')
+def _pullbookmarkbundle1(pullop):
+    """fetch bookmark data in bundle1 case
+
+    If not using bundle2, we have to fetch bookmark before doing changesets
+    discovery to reduce the change and impact of race condition."""
+    if not _canusebundle2(pullop): # all bundle2 server now support listkeys
+        pullop.remotebookmarks = pullop.remote.listkeys('bookmarks')
+
+
 @pulldiscovery('changegroup')
 def _pulldiscoverychangegroup(pullop):
     """discovery phase for the pull
 
     Current handle changeset discovery only, will change handle all discovery