remotefilelog and fastannotate already use a connection pool to share and reuse
connections. Treemanifest often does ondemand downloading of trees, such as
during hg log -p, and would greatly benefit from reusing connections as well.
This patch makes the connectionpool and attribute of the repo object, instead of
the fileserverclient object, which allows treemanifest to make use of it easily.
Differential Revision: https://phab.mercurial-scm.org/D1454
This fixes blackbox.log to not have two messages on the same line. This might be
undesirable if there's some other system using ui.log and this was *expected* to
be creating a single line. In that case, this might instead be a feature request
for blackbox to not insert time/user/node/etc. if it's a consecutive log from
the same 'service'. Currently, the docstring for ui.log says "*msg should be a
newline-terminated format string to log", so this is bringing these uses in
line with that.
Sample blackbox.log without this fix:
2017/11/06 14:41:23 spectral @a659d684cdf40d442d38f1ea65ee618f8b21d4b6 (25545)> remote cache hit rate is 0 of 9 2017/11/06 14:41:23 spectral @a659d684cdf40d442d38f1ea65ee618f8b21d4b6 (25545)> Success2017/11/06 14:45:24 spectral @dcbd198c160cfc8fc6d4a877aa5ed9296f98ee3c (25545)> pythonhook-update: remotefilelog.wcpprefetch finished in 0.00 seconds
Use naming consistent with Mercurial conventions and call the result of
bin(id) a node.
Test Plan:
None
Differential Revision: https://phab.mercurial-scm.org/D766
When calling prefetch in remotefilelog, also prefetch lfs
files.
We are using the same hook mechanism that remotefilelog is already using
for LFS by having remotefilelog call into LFS.
Test Plan:
run tests on test-lfs-remotefilelog-prefetch.t
Differential Revision: https://phab.mercurial-scm.org/D732
Remove an unecessary return as the function has no return value
anyway and this is the last statement in the function.
Test Plan:
None
Differential Revision: https://phab.mercurial-scm.org/D729
D319 renamed `peer.pipe[ioe]` to `peer._pipe[ioe]`.
D320 removed `peer.batch`.
D331 renamed `subprocess` to `_subprocess`.
D336 renamed `_capabilities` to `capabilities`.
Let's be compatible with those changes.
Differential Revision: https://phab.mercurial-scm.org/D425
Summary:
The bug was the following:
If you run hg prefetch, then hg repack, then hg prefetch, the second prefetch
downloads the same things as the first prefetch, even though the items are
already in the pack file.
That happened because Mercurial only checked if loose files exist before
prefetching(it didnt consider pack files). Now it also checks pack files.
Test Plan: Added test case, tested manually on www
Reviewers: simonfar, durham
Reviewed By: durham
Subscribers: medson, mjpieters, #mercurial
Differential Revision: https://phabricator.intern.facebook.com/D5574070
Tasks: 20797710
Signature: t1:5574070:1502124107:34cfabcfdcb81da79aff565641c9002bfe548def
Summary: Removed 'missing' list variable as it is not needed
Test Plan: No. All tests pass
Reviewers: simonfar, durham, stash
Reviewed By: stash
Subscribers: stash, medson, mjpieters, #mercurial
Differential Revision: https://phabricator.intern.facebook.com/D5461432
Signature: t1:5461432:1500559459:9bca4b429807082dd9a2a2a0283ab7b23df82b70
Summary: Number of files fetched from server is now logged to scuba.
Test Plan:
* rm -fr $(hg showconfig remotefilelog.cachepath)/* to clean cache
* hg show #rev
* check Scuba table 'Hg Remotefilelog' to ensure that # of files fetched from server is logged
Reviewers: durham, simonfar
Reviewed By: simonfar
Subscribers: medson, mjpieters, #mercurial
Differential Revision: https://phabricator.intern.facebook.com/D5375753
Tasks: 19727278
Signature: t1:5375753:1499347945:97ecef74418ad2fcbabfcd850a37622b23551815
Summary:
Previously remotefilelog would open a connection and leave the getfiles command
running on that connection, so it didn't have to reopen the ssh connection each
time. We want to reuse this ssh connection for treemanifest and fastannotate, so
let's switch it to a pool model where the connection is kept open but the
getfiles command is not left open.
If an exception happens while the connection is out of the pool, it is discarded
instead of being added back to the pool.
Test Plan:
Ran the tests. The fastannotate tests changed to reflect the new way
the connectionpool allows use.
Reviewers: quark, #mercurial, mitrandir
Reviewed By: mitrandir
Subscribers: mitrandir, medson, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D5280323
Signature: t1:5280323:1497975420:e3ae1ee854a1afc90816502543a19ff36f59b497
Summary:
A future patch will move the pack wireprotocol to use bundle2. In this new world
we'll be given a file handle instead of a remote peer, so let's switch the
utility methods to work on a file handle instead.
Test Plan: Ran the tests
Reviewers: #mercurial, quark
Reviewed By: quark
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4924553
Signature: t1:4924553:1493050882:7a9ee8b282bf47ef393362dd0114d801dc2a68d5
Summary:
The pack wireprotocol will be useful for exchanging treemanifests, so let's
refactor it out to it's own file. There's a slight protocol change here, where
we terminate the response with 10 null bytes (2 for 0 length filename, 4 for 0
length data, 4 for 0 length history) instead of the original 2 null bytes (for
0 length filename) which didn't let us handle entries with '' as the name.
This pack exchange code isn't even used in production, since most remotefilelog
downloads are done via the lose file (getfile/getfiles) format.
Test Plan:
Ran the tests. Even though this code isn't used in production, the
prefetch and repack tests still cover it.
Reviewers: #mercurial, rmcelroy
Reviewed By: rmcelroy
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4860556
Signature: t1:4860556:1491853521:c3810a4a681606571354b270b957e8df0962c86a
Summary:
This adds ui.log() output for prefetch statistics. Extensions who hook into
ui.log() can now log this data to external metrics systems.
Test Plan:
Ran a hg prefetch with the config flags enabled, while ptailing the dev command
timer. Verified the result contained remotefilelogfetches*
```
CHGDISABLE=1 FB_HG_DIAGS=1 hg --config
extensions.remotefilelog=../fb-hgext/remotefilelog/ --config
sampling.key.remotefilelog.prefetch=perfpipe_dev_command_timers prefetch -r .~9
ptail -f perfpipe_dev_command_timers | grep durham
```
Reviewers: #mercurial, simonfar
Reviewed By: simonfar
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4711096
Signature: t1:4711096:1489591144:1c91a4fbd118a3c10c2a2c68391c9f5b0dbcedf3
Summary:
This makes the `_getfiles` batch size configurable. Let me know if some other config name will serve this purpose better.
**Reason for this fix**
Currently, `_getfile` will write 10000 lines of text into a pipe and only upon the success of this operation, will read file blobs from another pipe. Serving process will start writing file blobs into a second pipe as soon as it sees something in the first pipe. Second pipe's buffer will fill up as it is not read from by the client until client writes 10K file requests. 10K file requests fill the buffer of the first pipe and we have a deadlock.
Ideally, we should make client check whether it can write to the first pipe and if not, go and read from the second pipe, but that is a bigger fix.
Test Plan:
- run local tests, see them all passing
- except for `test-cstore.t`, but it fails for me without my changes as well
- this generally makes sense
Reviewers: #sourcecontrol, durham
Reviewed By: durham
Subscribers: durham, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4620152
Signature: t1:4620152:1488221258:04555177926d129c6ba41bc982ad4e913cb31b20
Summary:
On Windows, the default SSH client shipped with TortoiseHg is TortoisePlink.
It is very slow and for some reason, we did not experiment deadlocks using this
client. (My unverified hypothesis is that it may have an internal buffer to
manage large requests).
Using another SSH client such as MSYS ssh.exe (shipped with git Windows
installer) dramatically speeds up tranfers (~8 times). However, using this client
yields the deadlock problem.
NOTE: I don't know why it's not a problem on linux boxes.
The fix is to issue requests from a separate thread, so we don't enter a
deadlock on the client.
Summary:
Treemanifest had a bug where the pack files it created were 400 instead of 444.
This meant people sharing a cache on the same machine couldn't access them. In
most of the remotefilelog code we had set opener.createmode before creating the
pack but we forgot to for treemanifest.
This patch moves the opener creation and createmode setting into the mutable
pack class so we can't screw this up again.
Test Plan:
Tests that wrote to the packs before, now failed and had to be
updated to chmod the packs before writing to them.
Reviewers: #mercurial, rmcelroy
Reviewed By: rmcelroy
Subscribers: rmcelroy, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4411580
Tasks: 15469140
Signature: t1:4411580:1484321293:9aa78254677548a6dc2270c58cee0ec6f57dd089
Summary:
fastannotate will tell remotefilelog what nodes of a file is already known in
linelog + revmap, so remotefilelog will not prefetch those files. Previously,
fastannotate either prevents all prefetching or allows all prefetching, which
is sub-optimal.
fastannotate could now donate its sshpeer to remotefilelog, so remotefilelog
won't need to start another one (assuming they can share a same sshpeer,
could be turned off via config options). This should reduce run time if SSH
handshake is expensive.
fastannotate could now also steal sshpeer from remotefilelog, so fastannotate
won't need to start another one. Combined with the above change, there would
always be only one sshpeer shared by fastannotate and remotefilelog.
Test Plan: Modified existing tests
Reviewers: #sourcecontrol, durham
Reviewed By: durham
Subscribers: ikostia, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4325382
Signature: t1:4325382:1481933531:39d6344b2c076fbbff1f07997cd268e7cee25e80
Summary:
This fixed a user report (P56867550) where the error message does not get
shown correctly, because ResponseError takes two parameters.
Also did a minor change for the first ResponseError to comply the format.
Test Plan: `arc diff`
Reviewers: durham, #sourcecontrol, simonfar
Reviewed By: simonfar
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4280775
Signature: t1:4280775:1481016196:4762161be699e5d215ec86b2d1385493d977a2b6
I keep tripping over bugs where this angers some part of our
stack. It's dumb to even be fetching these, but it's harmless to skip
them, so issue a developer warning when we encounter one and refuse to
fetch it.
Summary:
In a future diff we will be introducing packs into .hg/store, so we need to
differentiate between cache packs and local packs. This patch renames
getpackpath to getcachepackpath. A future diff will add getlocalpackpath.
This exposed a pyflakes error, so we fix that too.
Test Plan: Ran the tests
Reviewers: #mercurial, quark
Reviewed By: quark
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4055815
Signature: t1:4055815:1477059415:e0221557bbeec6701820c826f00390d3a71cd2d3
Summary:
This fixes all the pyflaks and module errors for the main remotefilelog
code base.
Test Plan: ./run-tests.py test-check* test-remotefilelog*
Reviewers: #mercurial, quark
Reviewed By: quark
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4055537
Signature: t1:4055537:1477049663:ee904d311d17d3659e055e2c109c68c9023cfd1f
Summary:
Adds the initial extension that sets up the ctreemanifest. It currently relies
on the fastmanifest extension to hook into all the manifest APIs to construct
ctreemanifests.
Test Plan:
In a future patch, I was able to run 'hg manifests' on a commit and
have it return the manifest contents by reading the treemanifest.
Reviewers: #fastmanifest, ttung
Reviewed By: ttung
Subscribers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3755327
Signature: t1:3755327:1472114482:0c5862cba68ed4db643d28c2fae01f33f5352970
Summary:
764cd9916c94 recently introduced code that was unconditionally checking the
repo.includepattern and repo.excludepattern attributes on a local repository
without first checking if this is a shallow repository. These attributes only
exist on shallow repositories, causing "hg pull" to crash on non-shallow
repositories. This crash wouldn't happen in simple circumstances, since the
remotefilelog extension only gets fully set up once a shallow repository object
has been created, however when using chg you can end up with scenarios where a
non-shallow repository is used in the same hg process after a shallow one.
This refactors the code to now store the local repository object on the remote
peer rather than trying to store the individual shallow, includepattern, and
excludepattern attributes.
Overall this code does still feel a bit janky to me -- the rest of the peer API
is independent of the local repository, but the _callstream() wrapper cares
about the local repository being referenced. It seems like we should ideally
redesign the APIs so that _callstream() receives the local repository data as
an argument (or we should make the peer <--> local repository assocation more
formal and explicit if think it's better to force an association here).
Test Plan: Added a new test which triggered the crash, but passes with these changes.
Reviewers: ttung, mitrandir, durham
Reviewed By: durham
Subscribers: net-systems-diffs@, yogeshwer
Differential Revision: https://phabricator.intern.facebook.com/D3756493
Tasks: 12823586
Signature: t1:3756493:1471971600:9666e9c31bf59070c3ace0821d47d322671eb5b1
Summary:
We actually have no idea what "hg.peer" will return and should check
if it has a "cleanup" method or not before wrapping it.
Test Plan: Code Review
Reviewers: ttung, #mercurial, rmcelroy
Reviewed By: rmcelroy
Differential Revision: https://phabricator.intern.facebook.com/D3703201
Signature: t1:3703201:1470924859:852eaf275c89ceced285a4b74d09938e489d9ee0
Blame Revision: D3685587
Summary:
The deadlock happens in sshpeer.cleanup:
```
# sshpeer.cleanup
def cleanup(self):
if self.pipeo is None:
return
self.pipeo.close() # [1]
self.pipei.close()
try:
# read the error descriptor until EOF
for l in self.pipee: # [2]
self.ui.status(_("remote: "), l)
except (IOError, ValueError):
pass
self.pipee.close()
```
Notes:
1. Normally, this closes "stdin" of the server-side ssh process so
the "ssh" process running server-side will detect EOF and end.
However, with workers calling "os.fork", there could be other
processes keeping "pipeo" open thus "ssh" won't receive EOF.
2. Deadlock happens here, if the ssh process cannot get EOF and
does not end itself, thus does not close its stderr.
The dead lock happens with these steps:
1. "hg update ..." starts. Let's call it "the master process".
2. Memcache miss happens, and a sshpeer is started by fileserverclient
pipei, pipeo, pipee get created.
3. Workers start. They inherit pipei, pipeo, pipee.
4. The master process wait for the workers without closing pipeo.
5. The workers are at the "cleanup" method.
6. The server-side "ssh" reading its stdin hangs because the master
process hasn't close pipeo, thus no EOF to its stdin.
7. The server-side "ssh" process never ends. It does not close its
stderr.
8. The workers reading from pipee never end. They never get EOF
because the server-side "ssh" won't close its stderr.
9. The master process waiting for workers never never completes.
Because workers won't exit before they read all pipee.
The patch closes pipee for forked processes to address the issue.
Ideally, we want this in sshpeer.py because it could in theory
affect other sshpeer use-cases. But let's do it for remotefilelog
for now since remotefilelog is currently the only victim of this
deadlock pattern.
Test Plan:
Add some extra debugging logs. Check the wrapped `_cleanup` gets called
and things work normally.
Reviewers: #mercurial, ttung, durham
Reviewed By: durham
Differential Revision: https://phabricator.intern.facebook.com/D3685587
Tasks: 12563156
Signature: t1:3685587:1470688591:0f4f97508699b273e17df867898d65205ee52434
Calling wrapfunction on the remotefilepeer(sshpeer) object in exchangepull
function introduces a reference cycle. Hence, this object will not be deleted
until the process dies. This is not a big issue for processes having a short
lifetime(e.g. lauched by command line.)
However, for persistent processes (e.g. TortoiseHg), this can lead to multiple
lingering ssh connections to the server(actually one by pull operation).
The fix is to not wrap the remotefilepeer._callstream. This method is defined
right into the remotefilepeer object. The required repo data is made available
in the remotefilepeer object by monkeypatching this object in the exchangepull
function.
Summary:
The pack path logic did not use the correct unix group when
remotefilelog.cachegroup was specified. This fixes that.
Test Plan:
I manually tested it by deleting a pack dir and running repack. This
is hard to create an automated test for since the feature isn't really cross
platform, and we don't have a way to know what groups they have on their
machine.
Reviewers: #sourcecontrol, ttung, rmcelroy
Reviewed By: rmcelroy
Differential Revision: https://phabricator.intern.facebook.com/D3400756
Tasks: 11584114
Signature: t1:3400756:1465342537:ed023f6dc830117df5e85e294a41486f072714c9
The iterbatch() handling added in f93aa99d4f1e (fileserverclient: use
new iterbatch() method, 2016-03-22) was broken by 31e88bf6faf0 (store:
change fileserviceclient to write via new store, 2016-04-04). Fix it
by copying the pattern introduced elsewhere in that change.
On windows the grp module is not present, so we need to avoid importing it. This
means the shared group feature of remotefilelog is not supported on windows.
Summary:
Previously the fileserverclient logic only checked the data store when
determining whether to prefetch a given key or not. This meant that if the file
was in the data store but not the history store, it could result in a history
lookup failing to prefetch.
The fix is to separate the notions of data and history in the prefetch logic. By
default we just check the data store like before, but an optional argument
allows us to specify checking the history store as well (and we change the
remotemetadatastore to pass that argument).
Test Plan:
Ran the tests. Also ran the repack scenario in my large repo that
reproed the issue. In another diff, I'm going to come back and add a suite of
tests around the various repack permutations that I've seen cause issues.
Reviewers: #mercurial, ttung, mitrandir
Reviewed By: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3277239
Signature: t1:3277239:1463085690:49ad478048cd13836b60f7ac9190e2294f5e9c64
Summary:
Add a simple progress bar on the client when receiving a pack from the
server.
Test Plan: Ran it
Reviewers: #mercurial, ttung, mitrandir
Reviewed By: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3277265
Signature: t1:3277265:1463085643:aa0960092958c4f56a6d1c3a3901348dba48aa91
Summary:
This adds a new wire protocol command to allow clients to request a set
of file contents and histories from the server and receive them in pack format.
It's pretty simple and always returns all the history for every node requested
(which is a bit overkill), but it's labeled v1 and we can iterate on it.
Test Plan: Added a test
Reviewers: #mercurial, ttung, mitrandir
Reviewed By: mitrandir
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3277212
Signature: t1:3277212:1463421279:459cc84265502175b47df293647aab7e7a830185
Summary:
Instead of hard coding the list of stores in each union store, let's make it a
list and just test each store in order. This will allow easily adding new stores
and reordering the priority of the existing ones.
Also fix the remote store's contains function. 'contains' is the old name, and
it now needs to be getmissing in order to fit the store contract.
Test Plan: ran the tests
Reviewers: #sourcecontrol, ttung, rmcelroy
Reviewed By: rmcelroy
Differential Revision: https://phabricator.fb.com/D3205314
Signature: t1:3205314:1461606028:3a513ac82c5de668a7e40bbf7cc88d8754e2f0bb
Summary:
A future patch is going to change the union store to just contain an ordered
list of stores. Therefore we need a special spot to record which store is the
one that should receive writes.
Test Plan: ran the tests
Reviewers: #sourcecontrol
Differential Revision: https://phabricator.fb.com/D3205307
Summary: Fix failures found by check-code.
Test Plan: Ran the tests
Reviewers: #sourcecontrol, ttung
Reviewed By: ttung
Differential Revision: https://phabricator.fb.com/D3221369
Signature: t1:3221369:1461648197:185cbbba61a9d1a7a1beacd64153185d0d0826ed
Summary:
We've received a few complaints that receivemissing is throwing corrupt data
exceptions. My best guess is that we're not receiving all of the data for some
reason. Let's add an assertion to ensure all the data is present, so we can
narrow it down to a connection issue instead of actual corrupt data.
Test Plan: Ran the tests
Reviewers: #sourcecontrol, ttung
Differential Revision: https://phabricator.fb.com/D3136203
This was meant to be part of the previous stack of commits, but I pushed the
wrong stack. This patch addresses a number of code review feedback points, the
most visible being to remain 'contains' to something else (in this case
'getmissing').
The old way of fetching from the server required the base store api expose a way
for outside callers to add fetch handlers to the store. This exposed some of the
underlying details of how data is fetched in an unnecessary way and added an
awkward subscription api.
Let's just treat our remote caches as another store we can fetch from, and
require that the over arching configure logic (in shallowrepo.py) can connect
all our stores together in a union store.
Now that we have the new store abstraction, and now that remotefilelog.py writes
via it, let's also make fileserverclient write to the store via that API.
This required some refactoring of how receive missing worked, so we could pass
the filename down, as that is required for writing to the store.
