Summary:
Update the HgImporter code to check the return value from
`folly::writevFull()`
Reviewed By: wez
Differential Revision: D9507289
fbshipit-source-id: 00985f50ebb4717f7b97a199bf9efba6b4f5f628
Summary:
Previously the HgImporter code was not checking the return value of
`folly::readFull()`. This function does not throw exceptions: it may return
-1 on error or a partial number of bytes read. Previously the HgImporter code
did not perform any checking of the result from this function.
Reviewed By: wez
Differential Revision: D9507287
fbshipit-source-id: 80d8bdb54375eb287d1473675fffb48c4cd34c56
Summary:
These methods were only used in the test utility in one location. I plan to
clean up how the HgImporter code reads from the underlying python process, and
it will be easier if these are non-static methods.
This removes the ability from the test utility to read from a pre-computed
manifest data file. I don't think we care too much about this functionality
any more--most of our repositories are exclusively using treemanifest mode now
anyway.
Reviewed By: wez
Differential Revision: D9507290
fbshipit-source-id: 97fe3fc9d8574c97a41328ed42e7afdacb3734d1
Summary:
Add a wrapper class around HgImporter that can catch errors communicating with
the underlying `hg_import_helper.py` script. If something goes wrong
communicating with the python code we restart the import helper and retry
the operation once.
This code only retries on HgImporterError exceptions, and nothing throws these
at the moment. Therefore for now this should not result in any behavior
changes. In the future I will update our error handling to throw
HgImporterError when we fail to communicate with the underlying
hg_import_helper.py script.
Reviewed By: wez
Differential Revision: D9507291
fbshipit-source-id: 0167959df6292a577942e049576669fb6711ce76
Summary:
This is part of "the great r-valuification of folly::Future":
* This is something we should do for safety in general.
* Several of folly::Future's methods are lvalue-qualified even though they act as though they are rvalue-qualified, that is, they provide a postcondition that says, in effect, callers should act as though the method invalidated its `this` object (regardless of whether that invalidation was actual or logical).
* This violates the C++ principle to "Express ideas directly in code" (see Core Guidelines), and generally makes it more confusing for callers as well as hiding the actual semantics from tools (linters, compilers, etc.).
* This dichotomy and confusion has manifested itself by some failures around D7840699 since lvalue-qualification hides that operation's move-out semantics - leads to some use of future operations that are really not correct, but are not obviously incorrect.
* The goal of rvalueification is to make sure methods that are logically rvalue-qualified are actually rvalue-qualified, which forces callsites to acknowledge that rvalueification, e.g., `std::move(f).onError(...)` instead of `f.onError(...)`. This syntactic change in the callsites forces callers to acknowledge the method's rvalue semantics.
Codemod changes:
* expr.onError(...) ==> std::move(expr).onError(...) // if expr is not already an xvalue
* expr->onError(...) ==> std::move(*expr).onError(...)
Note: operator precedence of that last step is safe - no need to parenthesize `expr`. Reason: `->` binds more tightly than unary `*`.
Reviewed By: yfeldblum
Differential Revision: D9505757
fbshipit-source-id: de666f3a877313526d10f5d3569a1bbb2203f066
Summary:
D9494651 and D9415207 logically conflicted with each other, but did not have
conflicts at the source code level. They both landed independently which
resulted in a broken build even though CI had passed on their individual
diffs. This updates them to be compatible.
Reviewed By: wez, strager
Differential Revision: D9505360
fbshipit-source-id: 72743167d934d1d57044cda8d5a7af03346ab142
Summary:
Change HgImporter::importFileContents() to return a Blob object rather than
just the IOBuf.
This shouldn't have any behavior changes. However, it might help track down
some of the cases where we have seen files get incorrectly imported with empty
contents in some rare cases. We're not sure what is causing that, but
incorrectly using a moved-from IOBuf object would be one thing that might
produce this behavior. By changing this code to a `unique_ptr<Blob>` will
will eliminate the possibility of seeing empty buffer contents if some code
path is somehow ever able to use one of these objects after they have been
moved away from. (The code would end up with a null pointer in this case
rather than an empty buffer. If this ever does occur in practice a null
pointer would make the problem more obvious than an empty buffer.)
Reviewed By: chadaustin
Differential Revision: D9494651
fbshipit-source-id: 799cc1b1d0e2ac32ae2013dc80c84ec17cc1c491
Summary:
This updates the `hg_import_helper.py` code so that when importing files it
also includes the file length in the response, in addition to the response
contents itself. This information is redundant and exists purely to help
the receiving C++ code verify that the response is in fact completely valid.
This is implemented with a new command number, and `hg_import_helper.py` still
supports the old command number with the old behavior so that it will still
work if old edenfs daemon instances start new instances of
`hg_import_helper.py`.
Reviewed By: chadaustin
Differential Revision: D9494653
fbshipit-source-id: 34d1221b6048d6016b30fd7f606ca0b140552d80
Summary:
In D9486915 I updated the code to invoke `hg` from the local repository, but
this was invoking the telemetry wrapper, and this was still falling through
and running the system-installed `hg.real` binary rather than the from the
local repository. This fixes the code to also pass through the `HG_REAL_BIN`
and `CHG_BIN` environment variables when invoking the `hg` wrapper so that the
underling real hg code is picked up from the current repository as wel.
Reviewed By: chadaustin
Differential Revision: D9494271
fbshipit-source-id: 89c5154f501536b9d7a43d472b1204f10e5dc664
Summary: This commit makes eden ask Mononoke API Server first when it is fetching blobs.
Reviewed By: chadaustin
Differential Revision: D9415207
fbshipit-source-id: 1146a9b6295e8ddb15e4bcb098379ea75886baa5
Summary:
The desired end state for how next inode number gets passed across
takeover. This should not land as is - the prior diffs need to land
and wait for a while until everyone transitions.
Reviewed By: simpkins
Differential Revision: D8195550
fbshipit-source-id: 2fc40f881cc1a331df95ef99f7e9e4f2e69c8643
Summary:
Prior to this diff, to read the inode metadata from an InodeBase, you
had to cast it to a FileInode or TreeInode. Add a public getMetadata
call that acquires the appropriate lock depending on whether it's a
FileInode or TreeInode.
Reviewed By: simpkins
Differential Revision: D9327180
fbshipit-source-id: 66594fb639f1ea72a0e48dcb79234e14d87ec4a2
Summary:
Update the TARGETS file for eden/fs/store/hg/test to make sure that they
explicitly specify EDEN_HG_BINARY in the environment so that the test code
will run against hg from the local repository rather than the system-installed
mercurial.
Also update testharness/HgRepo.cpp to use this environment variable when
searching for the hg binary.
Reviewed By: chadaustin, wez
Differential Revision: D9486915
fbshipit-source-id: f8f8a1acde9e8b0a032a026fa477b21c70afacbc
Summary: The error message for FLAGS_etcEdenDir should contain its value.
Reviewed By: chadaustin
Differential Revision: D9484715
fbshipit-source-id: edecf42c74830a672ab9b3cdfac357b9abdedaf8
Summary:
Fix the code in `eden_dirstate._call_match_callbacks()` to correctly match
ignored symlinks that are explicitly listed in the matcher's file list.
It looks like this was just a mistake in the original code on my part: I
intended to check both `S_ISREG()` and `S_ISLNK()` but instead incorrectly put
the check for `S_ISREG()` twice.
Reviewed By: wez
Differential Revision: D9476524
fbshipit-source-id: 67e0fa7c2fbaac97598a8e2d028c9ef0999ed88a
Summary: Modifies a few callsites in Eden that indirect through a separately allocated lambda to replace Future::then with Future::thenValue.
Reviewed By: yfeldblum
Differential Revision: D9485137
fbshipit-source-id: 84fa03a736a0faa162372e221625d82813a68620
Summary:
When we load an empty blob from the LocalStore double check with the
BackingStore to confirm that it should actually be empty.
We have seen multiple instances of files being incorrectly imported as empty.
So far this error has always been fixed by a re-import. We still haven't
tracked down the root cause, but this change should help workaround the issue
by ensuring that we double check the file contents before returning the data.
Reviewed By: chadaustin
Differential Revision: D9476522
fbshipit-source-id: 6d57cf15c42736ecbcb106a731259b77db06d8f1
Summary:
`F14NodeMap` has proven to be a safe and more performant drop-in replacement
for `std::unordered_map`. With the ability to do heterogeneous lookup and
mutation, we no longer need `StringKeyedUnorderedMap`, whose main purpose was
to avoid unnecessary string copies during lookups and inserts.
Reviewed By: nbronson
Differential Revision: D9124737
fbshipit-source-id: 81d5be1df9096ac258a3dc1523a6a0f5d6b9e862
Summary:
D9389865 changed the rel_path variable from a string to a pathlib.Path object.
However it missed a couple places where the path object needed to be converted
to bytes.
Reviewed By: chadaustin
Differential Revision: D9476525
fbshipit-source-id: 2dc69f5a0378ffbc02b2724a097604df288683c9
Summary:
Overall plan to modify Future<T>::then to be r-value qualified and use Future<T>::thenTry or Future<T>::thenValue.
The goal is to disambiguate folly::Future and to improve type and lifetime safety of Future and its methods.
Codemod:
* future<T>.then(callable with operator()(not-a-try)) to future<T>.thenValue(callable with operator()(not-a-try)).
* future<T>.then(callable with operator()()) to future<T>.thenValue(callable with operator()(auto&&)).
* future<T>.then(callable with operator()(auto)) to future<T>.thenValue(callable with operator()(auto)).
Reviewed By: chadaustin
Differential Revision: D9443286
fbshipit-source-id: be712b58b92dc7422f128713deaf6f46b29b36ce
Summary:
Update the `_get_watch_roots_for_watchman()` check so that it does not fail if
there is no `roots` entry in the output from `watchman watch-list`
Reviewed By: wez
Differential Revision: D9445138
fbshipit-source-id: e066375db3e963d08a91aa2e257d6c4fdd18da74
Summary:
Existing timer precision (milliseconds) is too coarse to
debug many operations, it will often round down to 0ms. I didn't see
any rationale for not using something more precise here.
Reviewed By: chadaustin
Differential Revision: D9437556
fbshipit-source-id: 6a79431c5a976e933ecda228aa5386c07b8fb668
Summary:
Add an `--extract-to` flag to `eden debug overlay` to ask it to copy inode
data out of the overlay. When used with a file inode this copies the file
contents. When used with a directory inode this copies the directory
structure and file contents for all materialized files under that directory.
This makes it easier to extract user's untracked and modified files out of the
overlay when recovering state after an unclean filesystem shutdown.
Reviewed By: wez
Differential Revision: D9445559
fbshipit-source-id: 9abcf82695014d79f248ba5fdb09727a49f3098a
Summary:
Allow callers to explicitly pass in the location of an overlay storage
directory, rather than requiring that this be found from a running Eden
instance.
This makes it easier to make copies of an overlay for later offline debugging.
Reviewed By: wez
Differential Revision: D9445560
fbshipit-source-id: b1f2ee0dc8bfca80dcc02c57c348f65599ab86e2
Summary:
Update all of the `eden debug` commands to use the newer `find_checkout()`
code rather than older `get_mount_path()` function.
The `find_checkout()` code makes sure that the EdenInstance actually points to
the correct edenfs instance for this checkout, and also works with checkouts
that are not currently mounted. In particular this allows
`eden debug overlay` to examine the overlay state even when the checkout is
not currently mounted.
Reviewed By: wez
Differential Revision: D9389865
fbshipit-source-id: 00578519d4805157a30c9b39abee9838925e8e76
Summary:
This adds a new `find_eden()` method to the Eden CLI code which can look up
information about the correct EdenCheckout and EdenInstance given a path.
In the future most CLI commands should switch to use this function over the
current `get_eden_instance()` and `get_mount_path()` methods. These older
APIs only work to find currently mounted checkouts, which is inadequate for
commands like `fsck` and some other debug commands that want to be able to
operate on unmounted checkouts.
This new logic is able to correctly find checkout information even for
unmounted checkouts. If the checkout is managed by an edenfs instance running
out of a non-default location it also correctly finds the state information
for Eden, rather than potentially using the wrong Eden state directory.
Reviewed By: wez
Differential Revision: D9385821
fbshipit-source-id: a6638f3c3817a595a7b7979c5cd218a2d7400f51
Summary: If the OS is Windows it will expect the in-fd and out-fd as Windows handles and convert them to CRT fds. This will not change the behavior if it's not Windows.
Reviewed By: wez
Differential Revision: D9344414
fbshipit-source-id: 8b30b57d979dc45af06676979e75e00b02d2b89b
Summary:
This diff is first in the series to make Eden work on Windows. It includes:
1. HG backing store and Object store, which provides the capability to talk to mercurial and fetch the file and folder contents on Windows.
2. Subprocess and Pipe definition for Windows.
3. The Visual studio solution and projects files to compile Eden and scm datapack.
Few Important points:
1. Most of the changes to existing code is done under a macro EDEN_WIN so that it doesn't impact on other platform.
2. Sqlite is used for caching the fetched contents. We are not using Rocksdb on Windows.
3. The main function only calls some test code and exit after printing the output.
4. The initializeMononoke code is disabled for Windows because it needs Proxygen to talk HTTP. Will enable this once I get Proxygen and other dependencies working.
5. HgImporter pass Windows handles to hg_import_helper as command line args. The code to convert these handles into fds is in a separate diff.
Reviewed By: wez
Differential Revision: D8653992
fbshipit-source-id: 52a3c3750425fb92c2a7158c2c214a9372661e13
Summary:
As a prerequisite to running inode unloading code in a background
queue, use a deterministic executor in TestMount to make sequencing in
unit tests reliable.
Reviewed By: simpkins
Differential Revision: D9323878
fbshipit-source-id: 0b85632c1637a8cf83d6f238675e5b6bbb6923c7
Summary:
Update the hg_import_helper.py code to catch exceptions from `repo.close()`
when we are re-opening the repository after an error. Also set `self.repo` to
`None` before we start, so that it will be left as None if anything goes wrong
either closing or re-opening the repository.
I ran into a situation where `repo.close()` threw an error, and previously
this would leave `hg_import_helper.py` stuck in a bad state since it would
still be pointing to the old repo object. The next time it received a command
it would fail and try to call `self.repo.close()` again, which would still
fail since the repository was already in a halfway closed state.
Now the code will always forget about the old repository object and create a
new object.
Reviewed By: wez
Differential Revision: D9419109
fbshipit-source-id: 15bb296ba19d9d3d2a2b90169bf25b0e8e197c1f
Summary:
Fix the output of `eden debug journal` after changing the Thrift API
to pass paths in bytes instead of strings.
Reviewed By: simpkins
Differential Revision: D9403416
fbshipit-source-id: acd9d0f8b241d11295a48fa11f9cc59bd86381f2
Summary: This commit let `HgImporter` pick up configuration values from `EdenConfig`.
Reviewed By: chadaustin
Differential Revision: D9346293
fbshipit-source-id: cb63f7d13a86058e9bf076eddb52212560a64cb1
Summary:
While destroying JournalDelta entries, unzip the chain as we go and
destroy each entry one by one. This prevents the stack from
overflowing on long JournalDelta chains and should fix some flaky
tests.
Reviewed By: wez
Differential Revision: D9355365
fbshipit-source-id: 31af124d318ca5d7a84314b707e1b3c71b2ccaa9
Summary:
For O(1) stack space JournalDelta destruction, we need to check
whether a reference-counted pointer has a unique refcount. It looks at
first glance like shared_ptr<T>::unique() gives us that, but it uses a
relaxed load and is now deprecated.
Sadly, the easiest thing is to make a new smart pointer class. (It
happens to be slightly more efficient, requiring one fewer heap
allocation per entry in the chain.)
Reviewed By: simpkins
Differential Revision: D9355314
fbshipit-source-id: 8c782ba9e0ec27fae90325079c199e1b82df88fa
Summary:
This is part of "the great r-valuification of folly::Future":
* This is something we should do for safety in general.
* Several of folly::Future's methods are lvalue-qualified even though they act as though they are rvalue-qualified, that is, they provide a postcondition that says, in effect, callers should act as though the method invalidated its `this` object (regardless of whether that invalidation was actual or logical).
* This violates the C++ principle to "Express ideas directly in code" (see Core Guidelines), and generally makes it more confusing for callers as well as hiding the actual semantics from tools (linters, compilers, etc.).
* This dichotomy and confusion has manifested itself by some failures around D7840699 since lvalue-qualification hides that operation's move-out semantics - leads to some use of future operations that are really not correct, but are not obviously incorrect.
* The goal of rvalueification is to make sure methods that are logically rvalue-qualified are actually rvalue-qualified, which forces callsites to acknowledge that rvalueification, e.g., `std::move(f).ensure(...)` instead of `f.ensure(...)`. This syntactic change in the callsites forces callers to acknowledge the method's rvalue semantics.
This diff started as a Codemod, then required manual fixes. Here were the codemod steps:
* expr.ensure(...) ==> std::move(expr).ensure(...) // if expr is not already an xvalue
* expr->ensure(...) ==> std::move(*expr).ensure(...)
Note: operator precedence of that last step is safe - no need to parenthesize `expr`. Reason: `->` binds more tightly than unary `*`.
Reviewed By: yfeldblum
Differential Revision: D9332070
fbshipit-source-id: 882121fe82c05fdb196ce676db686b6bc254974b
Summary:
Now that timestamps are read from the inode metadata table, and users
aren't likely to run a pre-metadata-table version, the timestamp data
in the overlay header's no longer needs to be written. So remove that
code which has the bonus of making unloading faster.
Reviewed By: wez
Differential Revision: D9318044
fbshipit-source-id: 27a9a9ee954003940209819466932237a81f8929
Summary:
The Eden CLI allowed setting the `EDEN_CONFIG_DIR` environment variable to
control where to find the `.eden` state directory, as an alternative to the
`--config-dir` command line argument. However, nothing currently appears to
use this variable. Therefore remove this functionality for now to help
simplify the code.
Reviewed By: wez
Differential Revision: D9355514
fbshipit-source-id: c64afb54f599924945573b07bc6d91b346978ea8
Summary: Fix a few minor issues that `mypy --strict` complains about.
Reviewed By: wez
Differential Revision: D9355653
fbshipit-source-id: af63825721fc964b7713df68e8618b595c91561d
Summary:
In the CLI, rename the "Config" class to "EdenInstance". This class
represents all state about a particular edenfs instance. It provides APIs for
making thrift calls to edenfs, for creating and destroying checkouts, and
generally does much more than just managing configuration.
Renaming it to "EdenInstance" also helps clarify that it is distinct from the
configuration tracked in the user's ~/.edenrc and the /etc/eden directory.
This change purely renames internal class and variable names and should not
affect any user-visible functionality.
Reviewed By: wez
Differential Revision: D9355515
fbshipit-source-id: ba5d4c3b753c6eb12a3783306dcd29e85fea3f52
Summary:
This is part of "the great r-valuification of folly::Future":
* This is something we should do for safety in general.
* Several of folly::Future's methods are lvalue-qualified even though they act as though they are rvalue-qualified, that is, they provide a postcondition that says, in effect, callers should act as though the method invalidated its `this` object (regardless of whether that invalidation was actual or logical).
* This violates the C++ principle to "Express ideas directly in code" (see Core Guidelines), and generally makes it more confusing for callers as well as hiding the actual semantics from tools (linters, compilers, etc.).
* This dichotomy and confusion has manifested itself by some failures around D7840699 since lvalue-qualification hides that operation's move-out semantics - leads to some use of future operations that are really not correct, but are not obviously incorrect.
* The goal of rvalueification is to make sure methods that are logically rvalue-qualified are actually rvalue-qualified, which forces callsites to acknowledge that rvalueification, e.g., `std::move(f).unit(...)` instead of `f.unit(...)`. This syntactic change in the callsites forces callers to acknowledge the method's rvalue semantics.
Codemod changes:
* expr.unit(...) ==> std::move(expr).unit(...) // if expr is not already an xvalue
* expr->unit(...) ==> std::move(*expr).unit(...)
Note: operator precedence of that last step is safe - no need to parenthesize `expr`. Reason: `->` binds more tightly than unary `*`.
Reviewed By: LeeHowes
Differential Revision: D9347419
fbshipit-source-id: 2773365f3793d977f2bad1c0a85ef394633e7d2c
Summary:
Watchman's Eden integration has a bug where the combination of
Watchman querying Eden for overlapping delta ranges ("give me changes
between X and Y, now changes between X+1 and Y+1") and Eden eliding
redundant change events ("add-modify-remove" -> []) results in
Watchman sometimes reporting that a file exists in its final
subscription update when it no longer does.
The fix is to never elide events, even for files that were added and
removed in the same sequence. To continue to support Watchman's `new`
flag, track whether a file existed at the beginning and end of a
journal delta.
Reviewed By: wez
Differential Revision: D9304964
fbshipit-source-id: f34c12b25f2b24e3a0d46fc94aa428528f4c5098
Summary:
The setcon() failure is not actionable or interesting, so don't log it
to stdout at Eden startup.
Reviewed By: wez
Differential Revision: D9344467
fbshipit-source-id: 68435c8f22c228f2fbb86f37c2b1874723934169
Summary:
Most of the time, the age cutoff for unloads is extraneous, so default
it to 0 in order to unload everything.
Reviewed By: wez
Differential Revision: D9329161
fbshipit-source-id: a241630ea5c069b6f9dafc9f021e39365b96b3bf
Summary: This commit adds two configurable value for setting path to client certificate and a flag to control Mononoke integration.
Reviewed By: chadaustin
Differential Revision: D9303157
fbshipit-source-id: 2f44d55d17b567655157a5f4b6f52e9468dda234
Summary:
To prevent the regression fixed by D8323051, make sure we run at least a couple
integration tests with the RocksDB LocalStore implementation.
Reviewed By: wez
Differential Revision: D8408390
fbshipit-source-id: 8fab4041ae39915d8be80e42814aab375c4acdda
Summary:
This prevents `hg status` from blowing up with a UTF-8 decode
error inside the generated thrift code.
Push safety concerns:
* This doesn't change the wire representation of the data
* Existing clients that believe it to be a string will continue to have
the same behavior
* Buck has its own copy of an older version of the thrift spec, so it will
continue to work "OK".
* When buck resyncs with our thrift file, some changes will likely be needed
to convert the byte arrays to strings or paths or whatever is appropriate
for bucks internal API
Work "OK" above means that clients that currently believe that `string` is
utf-8 encoded will have a runtime error if we ever send them a path that
is not utf-8. This is the behavior prior to this diff and will continue
to be the behavior for clients (like buck) that have an older version
of the thrift file.
Reviewed By: simpkins
Differential Revision: D9270843
fbshipit-source-id: b01135aec9152aaf5199e1c654ddd7f61c03717e
Summary:
This is a recurring issue for folks that have unstable systems
that either OOM or are hard rebooted.
Provide advice on how to resolve the issue.
Reviewed By: simpkins
Differential Revision: D9275963
fbshipit-source-id: d746b2cce56716d738304430e484784baf49eb29
Summary:
The root inode is particularly important, so always call `fdatasync()` on its
changes in the overlay before committing them.
This will hopefully reduce the number of cases we see where users have empty
or corrupt data for the root inode after hard rebooting their server. My
guess is that the root directory is being modified by hg or other tools
creating and removing temporary files in the root directory. If a change like
this is in progress when a hard reboot has been performed we risk data loss
without the `fdatasync()` call.
While Eden can still mostly serve the checkout data if other files or
directories are corrupt/missing in the overlay it currently is completely
unable to mount the checkout if the root overlay is corrupt. Therefore it
seems worth being more cautious about making sure that the root overlay data
is updated atomically.
Reviewed By: chadaustin, wez
Differential Revision: D9275852
fbshipit-source-id: b1e3eeb94ba670d0e2b52da4af7143d3ddbc919b
Summary:
eden doctor misreports the case that there are duplicate filewatcher
subscriptions for a Nuclide mount. Correctly report those cases as
duplicates instead of missing.
Reviewed By: wez
Differential Revision: D9228450
fbshipit-source-id: cd6fb3c3aa0c3c12f370c084ebc413eeff16ef02
Summary:
We have seen issues with Eden occasionally getting empty file contents back
from Mercurial. We have not been able to reproduce this issue directly
yet--the overall incidence rate relative to the number of users is fairly
small, but the problem is pretty problematic when it does occur.
This updates the hg_import_helper.py script to perform additional checking
when it finds a file that is empty.
Empty files that have never been modified or renamed all have the same file
revlog hash. If the rev hash in question is this known hash, we know that the
file is in fact empty. Otherwise, if remotefilelog is in use we check the
remotefilelog metadata to confirm the size, and we log an error if it is
non-zero. We then try re-opening the repository and re-importing the file.
These additional checks and log messages should hopefully help narrow down the
problem the next time we see a report of the issue. This should help us
identify if the remotefilelog metadata has the correct file size or not.
Reviewed By: wez
Differential Revision: D9260788
fbshipit-source-id: 29615b32632946cd319aa837bec3c68b757d3ee0