This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5137

streamclone: new server config and some API changes for narrow stream clones
ClosedPublic

Authored by pulkit on Oct 17 2018, 10:52 AM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGaf62936c2508: streamclone: new server config and some API changes for narrow stream clones
Summary

This patch introduces a new server config server.stream-narrow-clones which if
set to True will advertise that the server supports narrow stream clones.

This patch also pass on the includes and excludes from getbundle command to
streamclone generation code.

There is a test added to show that the includepats and excludepats are correctly
passed.

Upcoming patches will implement storage layer filtering for streamclones and
then we can remove the temporary error and plug in the whole logic together to
make narrow stream clones working.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Oct 17 2018, 10:52 AM
indygreg added a subscriber: indygreg.Oct 17 2018, 11:19 AM

I'll hold off formally accepting until the whole series is up. But this looks pretty good.

We need to land this in 4.8 or less the client-side feature detection logic will require a new server capability (or a bump of the version of the capability string).

mercurial/configitems.py
1017

Perhaps we should mark this as experimental via an inline comment and forego documenting the config option for now?

pulkit added a comment.Oct 17 2018, 11:24 AM
In D5137#76870, @indygreg wrote:

I'll hold off formally accepting until the whole series is up. But this looks pretty good.
We need to land this in 4.8 or less the client-side feature detection logic will require a new server capability (or a bump of the version of the capability string).

The whole series is up now. Although this is important for us, I realize there is just 1 day left for the freeze and I won't push hard for getting this pushed in this cycle and will graft these csets if looks good. I know you are on a vacation, so thanks for taking time and reviewing.

indygreg added a comment.Oct 17 2018, 11:26 AM

There are no children reviews of this one. Could you please set the parent-child relationships so the entire stack renders in the web UI?

(I'm assuming there are commits that follow this one - an unimplemented narrow stream clone feature doesn't seem very useful!)

pulkit added a comment.Oct 17 2018, 11:30 AM
In D5137#76876, @indygreg wrote:

There are no children reviews of this one. Could you please set the parent-child relationships so the entire stack renders in the web UI?
(I'm assuming there are commits that follow this one - an unimplemented narrow stream clone feature doesn't seem very useful!)

Should be good now.

martinvonz added a subscriber: martinvonz.Oct 17 2018, 6:45 PM
martinvonz added inline comments.
mercurial/configitems.py
1017

Makes sense, but I don't know how to do that, so I'll just rename it to experimental.server.stream-narrow-clones in flight (because we're close to the freeze)

martinvonz accepted this revision.Oct 17 2018, 6:50 PM
This revision is now accepted and ready to land.Oct 17 2018, 6:50 PM
martinvonz added inline comments.Oct 17 2018, 6:58 PM
mercurial/bundle2.py
1691–1692

I had to change these to kwargs.get() to get test-http.t and many others to pass. (I also fixed the typo in the comment above)

mercurial/help/config.txt
1851–1854 ↗(On Diff #12208)

I removed this per indygreg's comment

mercurial/streamclone.py
549

I changed this to "server does not support narrow stream clones" to match the error from bundle2.

Btw, why is it raised in two places? Is the one in bundle2 that actually happens? Maybe this should be a ProgrammingError then?

tests/test-narrow-clone-stream.t
38

I changed this to "server does not support narrow stream clones" to match the actual output (test was failing)

Closed by commit rHGaf62936c2508: streamclone: new server config and some API changes for narrow stream clones (authored by pulkit). · Explain WhyOct 17 2018, 7:25 PM
This revision was automatically updated to reflect the committed changes.
pulkit added a comment.Oct 18 2018, 5:21 AM

@martinvonz many thanks for changing things in flight. Any reason why D5138 and D5139 are not considered during review because with them, you won't have to change much things except the config knob to experimental.

Revision Contents
Changeset List

Diff 12227

mercurial/bundle2.py

'requesting stream data from servers not ' 'requesting stream data from servers not '
'advertising it; the client may be buggy')) 'advertising it; the client may be buggy'))
# Stream clones don't compress well. And compression undermines a # Stream clones don't compress well. And compression undermines a
# goal of stream clones, which is to be fast. Communicate the desire # goal of stream clones, which is to be fast. Communicate the desire
# to avoid compression to consumers of the bundle. # to avoid compression to consumers of the bundle.
bundler.prefercompressed = False bundler.prefercompressed = False
filecount, bytecount, it = streamclone.generatev2(repo) # get the includes and excludes
includepats = kwargs.get(r'includepats')
excludepats = kwargs.get(r'excludepats')
martinvonzUnsubmitted
Not Done

I had to change these to kwargs.get() to get test-http.t and many others to pass. (I also fixed the typo in the comment above)

martinvonz: I had to change these to `kwargs.get()` to get `test-http.t` and many others to pass. (I also…
narrowstream = repo.ui.configbool('experimental.server',
'stream-narrow-clones')
if (includepats or excludepats) and not narrowstream:
raise error.Abort(_('server does not support narrow stream clones'))
filecount, bytecount, it = streamclone.generatev2(repo, includepats,
excludepats)
requirements = _formatrequirementsspec(repo.requirements) requirements = _formatrequirementsspec(repo.requirements)
part = bundler.newpart('stream2', data=it) part = bundler.newpart('stream2', data=it)
part.addparam('bytecount', '%d' % bytecount, mandatory=True) part.addparam('bytecount', '%d' % bytecount, mandatory=True)
part.addparam('filecount', '%d' % filecount, mandatory=True) part.addparam('filecount', '%d' % filecount, mandatory=True)
part.addparam('requirements', requirements, mandatory=True) part.addparam('requirements', requirements, mandatory=True)
def buildobsmarkerspart(bundler, markers): def buildobsmarkerspart(bundler, markers):
"""add an obsmarker part to the bundler with <markers> """add an obsmarker part to the bundler with <markers>

mercurial/configitems.py

default=None, default=None,
) )
coreconfigitem('experimental', 'server.filesdata.recommended-batch-size', coreconfigitem('experimental', 'server.filesdata.recommended-batch-size',
default=50000, default=50000,
) )
coreconfigitem('experimental', 'server.manifestdata.recommended-batch-size', coreconfigitem('experimental', 'server.manifestdata.recommended-batch-size',
default=100000, default=100000,
) )
coreconfigitem('experimental.server', 'stream-narrow-clones',
default=False,
)
coreconfigitem('experimental', 'single-head-per-branch', coreconfigitem('experimental', 'single-head-per-branch',
default=False, default=False,
) )
coreconfigitem('experimental', 'sshserver.support-v2', coreconfigitem('experimental', 'sshserver.support-v2',
default=False, default=False,
) )
coreconfigitem('experimental', 'sparse-read', coreconfigitem('experimental', 'sparse-read',
default=False, default=False,
default=False, default=False,
) )
coreconfigitem('server', 'preferuncompressed', coreconfigitem('server', 'preferuncompressed',
default=False, default=False,
) )
coreconfigitem('server', 'streamunbundle', coreconfigitem('server', 'streamunbundle',
default=False, default=False,
) )
coreconfigitem('server', 'uncompressed', coreconfigitem('server', 'uncompressed',
indygregUnsubmitted
Not Done

Perhaps we should mark this as experimental via an inline comment and forego documenting the config option for now?

indygreg: Perhaps we should mark this as experimental via an inline comment and forego documenting the…
martinvonzUnsubmitted
Not Done

Makes sense, but I don't know how to do that, so I'll just rename it to experimental.server.stream-narrow-clones in flight (because we're close to the freeze)

martinvonz: Makes sense, but I don't know how to do that, so I'll just rename it to `experimental.server.
default=True, default=True,
) )
coreconfigitem('server', 'uncompressedallowsecret', coreconfigitem('server', 'uncompressedallowsecret',
default=False, default=False,
) )
coreconfigitem('server', 'validate', coreconfigitem('server', 'validate',
default=False, default=False,
) )

mercurial/streamclone.py

chunks = util.filechunkiter(fp, limit=size) chunks = util.filechunkiter(fp, limit=size)
for chunk in chunks: for chunk in chunks:
seen += len(chunk) seen += len(chunk)
progress.update(seen) progress.update(seen)
yield chunk yield chunk
finally: finally:
fp.close() fp.close()
def generatev2(repo): def generatev2(repo, includes, excludes):
"""Emit content for version 2 of a streaming clone. """Emit content for version 2 of a streaming clone.
the data stream consists the following entries: the data stream consists the following entries:
1) A char representing the file destination (eg: store or cache) 1) A char representing the file destination (eg: store or cache)
2) A varint containing the length of the filename 2) A varint containing the length of the filename
3) A varint containing the length of file data 3) A varint containing the length of file data
4) N bytes containing the filename (the internal, store-agnostic form) 4) N bytes containing the filename (the internal, store-agnostic form)
5) N bytes containing the file data 5) N bytes containing the file data
Returns a 3-tuple of (file count, file size, data iterator). Returns a 3-tuple of (file count, file size, data iterator).
""" """
# temporarily raise error until we add storage level logic
if includes or excludes:
raise error.Abort(_("server does not support narrow stream clones"))
martinvonzUnsubmitted
Not Done

I changed this to "server does not support narrow stream clones" to match the error from bundle2.

Btw, why is it raised in two places? Is the one in bundle2 that actually happens? Maybe this should be a ProgrammingError then?

martinvonz: I changed this to "server does not support narrow stream clones" to match the error from…
with repo.lock(): with repo.lock():
entries = [] entries = []
totalfilesize = 0 totalfilesize = 0
repo.ui.debug('scanning\n') repo.ui.debug('scanning\n')
for name, ename, size in _walkstreamfiles(repo): for name, ename, size in _walkstreamfiles(repo):
if size: if size:

tests/test-narrow-clone-stream.t

  • This file was added.
Tests narrow stream clones
$ . "$TESTDIR/narrow-library.sh"
Server setup
$ hg init master
$ cd master
$ mkdir dir
$ mkdir dir/src
$ cd dir/src
$ for x in `$TESTDIR/seq.py 20`; do echo $x > "f$x"; hg add "f$x"; hg commit -m "Commit src $x"; done
$ cd ..
$ mkdir tests
$ cd tests
$ for x in `$TESTDIR/seq.py 20`; do echo $x > "f$x"; hg add "f$x"; hg commit -m "Commit src $x"; done
$ cd ../../..
Trying to stream clone when the server does not support it
$ hg clone --narrow ssh://user@dummy/master narrow --noupdate --include "dir/src/f10" --stream
streaming all changes
remote: abort: server does not support narrow stream clones
abort: pull failed on remote
[255]
Enable stream clone on the server
$ echo "[server]" >> master/.hg/hgrc
$ echo "stream-narrow-clones=True" >> master/.hg/hgrc
Cloning a specific file when stream clone is supported
$ hg clone --narrow ssh://user@dummy/master narrow --noupdate --include "dir/src/f10" --stream
streaming all changes
remote: abort: server does not support narrow stream clones
abort: pull failed on remote
martinvonzUnsubmitted
Not Done

I changed this to "server does not support narrow stream clones" to match the actual output (test was failing)

martinvonz: I changed this to "server does not support narrow stream clones" to match the actual output…
[255]