Patchwork [1,of,2,V2] peer: introduce a limitedarguments attributes

login
register
mail settings
Submitter Pierre-Yves David
Date April 17, 2019, 5:07 p.m.
Message ID <6882a0101d389e27d697.1555520820@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/39700/
State Accepted
Headers show

Comments

Pierre-Yves David - April 17, 2019, 5:07 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1555516590 -7200
#      Wed Apr 17 17:56:30 2019 +0200
# Node ID 6882a0101d389e27d697bf2e9717de176f273309
# Parent  607a0de9bae31df526da75b68ab2853787d8c31e
# EXP-Topic discovery-speedup
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 6882a0101d38
peer: introduce a limitedarguments attributes

When set to True, it signal that the peer cannot receive too larges arguments
and that algorithm must adapt. This should only be True for http peer that does
not support argument passed as "post".

This will be useful to unlock better discovery performance in the next
changesets.

I am using a dedicated argument because this is not really a usual
"capabilities" things. An alternative approach would be to adds a
"large-arguments" to all peer, but the http peers. That seemed a bit too hacky
to me.
via Mercurial-devel - April 17, 2019, 5:17 p.m.
On Wed, Apr 17, 2019 at 10:07 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1555516590 -7200
> #      Wed Apr 17 17:56:30 2019 +0200
> # Node ID 6882a0101d389e27d697bf2e9717de176f273309
> # Parent  607a0de9bae31df526da75b68ab2853787d8c31e
> # EXP-Topic discovery-speedup
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
> 6882a0101d38
> peer: introduce a limitedarguments attributes
>
> When set to True, it signal that the peer cannot receive too larges
> arguments
> and that algorithm must adapt. This should only be True for http peer that
> does
> not support argument passed as "post".
>
> This will be useful to unlock better discovery performance in the next
> changesets.
>
> I am using a dedicated argument because this is not really a usual
> "capabilities" things. An alternative approach would be to adds a
> "large-arguments" to all peer, but the http peers. That seemed a bit too
> hacky
> to me.
>
> diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
> --- a/mercurial/httppeer.py
> +++ b/mercurial/httppeer.py
> @@ -382,6 +382,7 @@ class httppeer(wireprotov1peer.wirepeer)
>          self._path = path
>          self._url = url
>          self._caps = caps
> +        self.limitedarguments = caps is not None and 'httppostargs' not
> in caps
>

Is `'httppostargs' not in caps` enough to know that httppostargs will be
used for the "heads" request? As I said in an earlier email, I'm not sure
it will be respected for GET requests (and "heads" is a GET request,
right?). Did you check that this will not result in a GET request?


>          self._urlopener = opener
>          self._requestbuilder = requestbuilder
>
> @@ -750,6 +751,9 @@ class httpv2executor(object):
>
>  @interfaceutil.implementer(repository.ipeerv2)
>  class httpv2peer(object):
> +
> +    limitedarguments = False
> +
>      def __init__(self, ui, repourl, apipath, opener, requestbuilder,
>                   apidescriptor):
>          self.ui = ui
> diff --git a/mercurial/repository.py b/mercurial/repository.py
> --- a/mercurial/repository.py
> +++ b/mercurial/repository.py
> @@ -291,6 +291,10 @@ class ipeercommandexecutor(interfaceutil
>  class ipeerrequests(interfaceutil.Interface):
>      """Interface for executing commands on a peer."""
>
> +    limitedarguments = interfaceutil.Attribute(
> +        """True if the peer cannot receive large argument value for
> commands."""
> +    )
> +
>      def commandexecutor():
>          """A context manager that resolves to an ipeercommandexecutor.
>
> @@ -329,6 +333,8 @@ class ipeerv2(ipeerconnection, ipeercapa
>  class peer(object):
>      """Base class for peer repositories."""
>
> +    limitedarguments = False
> +
>      def capable(self, name):
>          caps = self.capabilities()
>          if name in caps:
>
Pierre-Yves David - April 17, 2019, 5:41 p.m.
On 4/17/19 7:17 PM, Martin von Zweigbergk wrote:
> 
> 
> On Wed, Apr 17, 2019 at 10:07 AM Pierre-Yves David 
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>> 
> wrote:
> 
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@octobus.net
>     <mailto:pierre-yves.david@octobus.net>>
>     # Date 1555516590 -7200
>     #      Wed Apr 17 17:56:30 2019 +0200
>     # Node ID 6882a0101d389e27d697bf2e9717de176f273309
>     # Parent  607a0de9bae31df526da75b68ab2853787d8c31e
>     # EXP-Topic discovery-speedup
>     # Available At https://bitbucket.org/octobus/mercurial-devel/
>     #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r 6882a0101d38
>     peer: introduce a limitedarguments attributes
> 
>     When set to True, it signal that the peer cannot receive too larges
>     arguments
>     and that algorithm must adapt. This should only be True for http
>     peer that does
>     not support argument passed as "post".
> 
>     This will be useful to unlock better discovery performance in the next
>     changesets.
> 
>     I am using a dedicated argument because this is not really a usual
>     "capabilities" things. An alternative approach would be to adds a
>     "large-arguments" to all peer, but the http peers. That seemed a bit
>     too hacky
>     to me.
> 
>     diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
>     --- a/mercurial/httppeer.py
>     +++ b/mercurial/httppeer.py
>     @@ -382,6 +382,7 @@ class httppeer(wireprotov1peer.wirepeer)
>               self._path = path
>               self._url = url
>               self._caps = caps
>     +        self.limitedarguments = caps is not None and 'httppostargs'
>     not in caps
> 
> 
> Is `'httppostargs' not in caps` enough to know that httppostargs will be 
> used for the "heads" request?

the `httppostargs` capabilities is http specific and cannot be used 
outside of the httppeer code (for example, ssh server has no argument 
side issue but still won't says they are httppostargs capable).

(note: we are talking about the `known` command heres, `heads` takes no 
arguments)

> As I said in an earlier email, I'm not 
> sure it will be respected for GET requests (and "heads" is a GET 
> request, right?). Did you check that this will not result in a GET request?

I am a bit confused about what you mean here. `known` will be included 
into a batch command (or standalone for quite older server).  that 
command will be get of post according to `httppostargs`, won't it?

I don't think I understood what your final question is. Can you try again?
via Mercurial-devel - April 17, 2019, 6:33 p.m.
On Wed, Apr 17, 2019 at 10:41 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 4/17/19 7:17 PM, Martin von Zweigbergk wrote:
> >
> >
> > On Wed, Apr 17, 2019 at 10:07 AM Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>
> > wrote:
> >
> >     # HG changeset patch
> >     # User Pierre-Yves David <pierre-yves.david@octobus.net
> >     <mailto:pierre-yves.david@octobus.net>>
> >     # Date 1555516590 -7200
> >     #      Wed Apr 17 17:56:30 2019 +0200
> >     # Node ID 6882a0101d389e27d697bf2e9717de176f273309
> >     # Parent  607a0de9bae31df526da75b68ab2853787d8c31e
> >     # EXP-Topic discovery-speedup
> >     # Available At https://bitbucket.org/octobus/mercurial-devel/
> >     #              hg pull
> >     https://bitbucket.org/octobus/mercurial-devel/ -r 6882a0101d38
> >     peer: introduce a limitedarguments attributes
> >
> >     When set to True, it signal that the peer cannot receive too larges
> >     arguments
> >     and that algorithm must adapt. This should only be True for http
> >     peer that does
> >     not support argument passed as "post".
> >
> >     This will be useful to unlock better discovery performance in the
> next
> >     changesets.
> >
> >     I am using a dedicated argument because this is not really a usual
> >     "capabilities" things. An alternative approach would be to adds a
> >     "large-arguments" to all peer, but the http peers. That seemed a bit
> >     too hacky
> >     to me.
> >
> >     diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
> >     --- a/mercurial/httppeer.py
> >     +++ b/mercurial/httppeer.py
> >     @@ -382,6 +382,7 @@ class httppeer(wireprotov1peer.wirepeer)
> >               self._path = path
> >               self._url = url
> >               self._caps = caps
> >     +        self.limitedarguments = caps is not None and 'httppostargs'
> >     not in caps
>

Should we limit them also if `caps is None`?

>
> >
> > Is `'httppostargs' not in caps` enough to know that httppostargs will be
> > used for the "heads" request?
>
> the `httppostargs` capabilities is http specific and cannot be used
> outside of the httppeer code (for example, ssh server has no argument
> side issue but still won't says they are httppostargs capable).
>
> (note: we are talking about the `known` command heres, `heads` takes no
> arguments)
>
> > As I said in an earlier email, I'm not
> > sure it will be respected for GET requests (and "heads" is a GET
> > request, right?). Did you check that this will not result in a GET
> request?
>
> I am a bit confused about what you mean here. `known` will be included
> into a batch command (or standalone for quite older server).  that
> command will be get of post according to `httppostargs`, won't it?
>
> I don't think I understood what your final question is. Can you try again?
>

Question 1: Do "heads" and "known" request normally use GET or POST
(without this patch, without batching)?

I believe the answer is "GET".

Question 2: Do "batch" requests use POST?

I believe the answer is "yes".

Question 3: Does httppostargs switch GET requests to POST requests?

I believe the answer is "no".

Together, that seems to mean that we now rely on the requests getting
batched for the httppostargs thing to take effect. Correct? If correct, it
seems we should also check if the server supports batching before we decide
to remove the limit. We should also add a comment to the with-block where
we call "heads" and "known" so someone knows that we rely on batching there
for it to work.


>
>
>
> --
> Pierre-Yves David
>
Joerg Sonnenberger - April 17, 2019, 8:35 p.m.
On Wed, Apr 17, 2019 at 11:33:45AM -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> Question 3: Does httppostargs switch GET requests to POST requests?
> 
> I believe the answer is "no".

See makev1commandrest in httppeer.py line 137ff. The answer is "yes, if
they exist".

Joerg
via Mercurial-devel - April 17, 2019, 9:38 p.m.
On Wed, Apr 17, 2019 at 2:33 PM Joerg Sonnenberger <joerg@bec.de> wrote:

> On Wed, Apr 17, 2019 at 11:33:45AM -0700, Martin von Zweigbergk via
> Mercurial-devel wrote:
> > Question 3: Does httppostargs switch GET requests to POST requests?
> >
> > I believe the answer is "no".
>
> See makev1commandrest in httppeer.py line 137ff. The answer is "yes, if
> they exist".
>

Thanks. That's a clear answer to my original question.


>
> Joerg
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
via Mercurial-devel - April 17, 2019, 10:39 p.m.
On Wed, Apr 17, 2019 at 2:38 PM Martin von Zweigbergk <martinvonz@google.com>
wrote:

>
>
> On Wed, Apr 17, 2019 at 2:33 PM Joerg Sonnenberger <joerg@bec.de> wrote:
>
>> On Wed, Apr 17, 2019 at 11:33:45AM -0700, Martin von Zweigbergk via
>> Mercurial-devel wrote:
>> > Question 3: Does httppostargs switch GET requests to POST requests?
>> >
>> > I believe the answer is "no".
>>
>> See makev1commandrest in httppeer.py line 137ff. The answer is "yes, if
>> they exist".
>>
>
> Thanks. That's a clear answer to my original question.
>

Given that, it seems safe to me. I've queued this series, thanks.


>
>
>>
>> Joerg
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
Pierre-Yves David - April 24, 2019, 6:35 a.m.
On 4/17/19 8:33 PM, Martin von Zweigbergk wrote:
> 
> 
> On Wed, Apr 17, 2019 at 10:41 AM Pierre-Yves David 
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>> 
> wrote:
> 
> 
> 
>     On 4/17/19 7:17 PM, Martin von Zweigbergk wrote:
>      >
>      >
>      > On Wed, Apr 17, 2019 at 10:07 AM Pierre-Yves David
>      > <pierre-yves.david@ens-lyon.org
>     <mailto:pierre-yves.david@ens-lyon.org>
>     <mailto:pierre-yves.david@ens-lyon.org
>     <mailto:pierre-yves.david@ens-lyon.org>>>
>      > wrote:
>      >
>      >     # HG changeset patch
>      >     # User Pierre-Yves David <pierre-yves.david@octobus.net
>     <mailto:pierre-yves.david@octobus.net>
>      >     <mailto:pierre-yves.david@octobus.net
>     <mailto:pierre-yves.david@octobus.net>>>
>      >     # Date 1555516590 -7200
>      >     #      Wed Apr 17 17:56:30 2019 +0200
>      >     # Node ID 6882a0101d389e27d697bf2e9717de176f273309
>      >     # Parent  607a0de9bae31df526da75b68ab2853787d8c31e
>      >     # EXP-Topic discovery-speedup
>      >     # Available At https://bitbucket.org/octobus/mercurial-devel/
>      >     #              hg pull
>      > https://bitbucket.org/octobus/mercurial-devel/ -r 6882a0101d38
>      >     peer: introduce a limitedarguments attributes
>      >
>      >     When set to True, it signal that the peer cannot receive too
>     larges
>      >     arguments
>      >     and that algorithm must adapt. This should only be True for http
>      >     peer that does
>      >     not support argument passed as "post".
>      >
>      >     This will be useful to unlock better discovery performance in
>     the next
>      >     changesets.
>      >
>      >     I am using a dedicated argument because this is not really a
>     usual
>      >     "capabilities" things. An alternative approach would be to adds a
>      >     "large-arguments" to all peer, but the http peers. That
>     seemed a bit
>      >     too hacky
>      >     to me.
>      >
>      >     diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
>      >     --- a/mercurial/httppeer.py
>      >     +++ b/mercurial/httppeer.py
>      >     @@ -382,6 +382,7 @@ class httppeer(wireprotov1peer.wirepeer)
>      >               self._path = path
>      >               self._url = url
>      >               self._caps = caps
>      >     +        self.limitedarguments = caps is not None and
>     'httppostargs'
>      >     not in caps
> 
> 
> Should we limit them also if `caps is None`?
> 
>      >
>      >
>      > Is `'httppostargs' not in caps` enough to know that httppostargs
>     will be
>      > used for the "heads" request?
> 
>     the `httppostargs` capabilities is http specific and cannot be used
>     outside of the httppeer code (for example, ssh server has no argument
>     side issue but still won't says they are httppostargs capable).
> 
>     (note: we are talking about the `known` command heres, `heads` takes no
>     arguments)
> 
>      > As I said in an earlier email, I'm not
>      > sure it will be respected for GET requests (and "heads" is a GET
>      > request, right?). Did you check that this will not result in a
>     GET request?
> 
>     I am a bit confused about what you mean here. `known` will be included
>     into a batch command (or standalone for quite older server).  that
>     command will be get of post according to `httppostargs`, won't it?
> 
>     I don't think I understood what your final question is. Can you try
>     again?
> 
> 
> Question 1: Do "heads" and "known" request normally use GET or POST 
> (without this patch, without batching)?
> 
> I believe the answer is "GET".

I tried to figure out that part, but could not figure it out. If you 
know where to look, I would be happy to get a pointer


> Question 2: Do "batch" requests use POST?
> 
> I believe the answer is "yes".

According to my code reading and test: yes.

> Question 3: Does httppostargs switch GET requests to POST requests?
> 
> I believe the answer is "no".

I have no idea. If not, we should probably do it.

> Together, that seems to mean that we now rely on the requests getting 
> batched for the httppostargs thing to take effect. Correct? If correct, 
> it seems we should also check if the server supports batching before we 
> decide to remove the limit.

I think so. I am fine with that. sending a follow up
via Mercurial-devel - April 24, 2019, 6:45 a.m.
On Tue, Apr 23, 2019 at 11:35 PM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 4/17/19 8:33 PM, Martin von Zweigbergk wrote:
> >
> >
> > On Wed, Apr 17, 2019 at 10:41 AM Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>
> > wrote:
> >
> >
> >
> >     On 4/17/19 7:17 PM, Martin von Zweigbergk wrote:
> >      >
> >      >
> >      > On Wed, Apr 17, 2019 at 10:07 AM Pierre-Yves David
> >      > <pierre-yves.david@ens-lyon.org
> >     <mailto:pierre-yves.david@ens-lyon.org>
> >     <mailto:pierre-yves.david@ens-lyon.org
> >     <mailto:pierre-yves.david@ens-lyon.org>>>
> >      > wrote:
> >      >
> >      >     # HG changeset patch
> >      >     # User Pierre-Yves David <pierre-yves.david@octobus.net
> >     <mailto:pierre-yves.david@octobus.net>
> >      >     <mailto:pierre-yves.david@octobus.net
> >     <mailto:pierre-yves.david@octobus.net>>>
> >      >     # Date 1555516590 -7200
> >      >     #      Wed Apr 17 17:56:30 2019 +0200
> >      >     # Node ID 6882a0101d389e27d697bf2e9717de176f273309
> >      >     # Parent  607a0de9bae31df526da75b68ab2853787d8c31e
> >      >     # EXP-Topic discovery-speedup
> >      >     # Available At https://bitbucket.org/octobus/mercurial-devel/
> >      >     #              hg pull
> >      > https://bitbucket.org/octobus/mercurial-devel/ -r 6882a0101d38
> >      >     peer: introduce a limitedarguments attributes
> >      >
> >      >     When set to True, it signal that the peer cannot receive too
> >     larges
> >      >     arguments
> >      >     and that algorithm must adapt. This should only be True for
> http
> >      >     peer that does
> >      >     not support argument passed as "post".
> >      >
> >      >     This will be useful to unlock better discovery performance in
> >     the next
> >      >     changesets.
> >      >
> >      >     I am using a dedicated argument because this is not really a
> >     usual
> >      >     "capabilities" things. An alternative approach would be to
> adds a
> >      >     "large-arguments" to all peer, but the http peers. That
> >     seemed a bit
> >      >     too hacky
> >      >     to me.
> >      >
> >      >     diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
> >      >     --- a/mercurial/httppeer.py
> >      >     +++ b/mercurial/httppeer.py
> >      >     @@ -382,6 +382,7 @@ class httppeer(wireprotov1peer.wirepeer)
> >      >               self._path = path
> >      >               self._url = url
> >      >               self._caps = caps
> >      >     +        self.limitedarguments = caps is not None and
> >     'httppostargs'
> >      >     not in caps
> >
> >
> > Should we limit them also if `caps is None`?
> >
> >      >
> >      >
> >      > Is `'httppostargs' not in caps` enough to know that httppostargs
> >     will be
> >      > used for the "heads" request?
> >
> >     the `httppostargs` capabilities is http specific and cannot be used
> >     outside of the httppeer code (for example, ssh server has no argument
> >     side issue but still won't says they are httppostargs capable).
> >
> >     (note: we are talking about the `known` command heres, `heads` takes
> no
> >     arguments)
> >
> >      > As I said in an earlier email, I'm not
> >      > sure it will be respected for GET requests (and "heads" is a GET
> >      > request, right?). Did you check that this will not result in a
> >     GET request?
> >
> >     I am a bit confused about what you mean here. `known` will be
> included
> >     into a batch command (or standalone for quite older server).  that
> >     command will be get of post according to `httppostargs`, won't it?
> >
> >     I don't think I understood what your final question is. Can you try
> >     again?
> >
> >
> > Question 1: Do "heads" and "known" request normally use GET or POST
> > (without this patch, without batching)?
> >
> > I believe the answer is "GET".
>
> I tried to figure out that part, but could not figure it out. If you
> know where to look, I would be happy to get a pointer
>
>
> > Question 2: Do "batch" requests use POST?
> >
> > I believe the answer is "yes".
>
> According to my code reading and test: yes.
>
> > Question 3: Does httppostargs switch GET requests to POST requests?
> >
> > I believe the answer is "no".
>
> I have no idea. If not, we should probably do it.
>
> > Together, that seems to mean that we now rely on the requests getting
> > batched for the httppostargs thing to take effect. Correct? If correct,
> > it seems we should also check if the server supports batching before we
> > decide to remove the limit.
>
> I think so. I am fine with that. sending a follow up
>

Joerg already replied to this thread saying that the answer to question 3
is "yes", so I don't think we need to do anything else.


>
>
> --
> Pierre-Yves David
>

Patch

diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
--- a/mercurial/httppeer.py
+++ b/mercurial/httppeer.py
@@ -382,6 +382,7 @@  class httppeer(wireprotov1peer.wirepeer)
         self._path = path
         self._url = url
         self._caps = caps
+        self.limitedarguments = caps is not None and 'httppostargs' not in caps
         self._urlopener = opener
         self._requestbuilder = requestbuilder
 
@@ -750,6 +751,9 @@  class httpv2executor(object):
 
 @interfaceutil.implementer(repository.ipeerv2)
 class httpv2peer(object):
+
+    limitedarguments = False
+
     def __init__(self, ui, repourl, apipath, opener, requestbuilder,
                  apidescriptor):
         self.ui = ui
diff --git a/mercurial/repository.py b/mercurial/repository.py
--- a/mercurial/repository.py
+++ b/mercurial/repository.py
@@ -291,6 +291,10 @@  class ipeercommandexecutor(interfaceutil
 class ipeerrequests(interfaceutil.Interface):
     """Interface for executing commands on a peer."""
 
+    limitedarguments = interfaceutil.Attribute(
+        """True if the peer cannot receive large argument value for commands."""
+    )
+
     def commandexecutor():
         """A context manager that resolves to an ipeercommandexecutor.
 
@@ -329,6 +333,8 @@  class ipeerv2(ipeerconnection, ipeercapa
 class peer(object):
     """Base class for peer repositories."""
 
+    limitedarguments = False
+
     def capable(self, name):
         caps = self.capabilities()
         if name in caps: