Summary:
The prior implementation of StaleMountsCheck filtered by path
and did not correctly handle seeing the same FUSE mount multiple times
in the mount table. This occurred when an Eden mount was created
underneath a bind mount.
Now it only unmounts mounts where st_dev does not match the st_dev of
any active mounts, and where st_uid matches the current user.
Reviewed By: simpkins
Differential Revision: D6787618
fbshipit-source-id: 24e0f156cb74822500d91205349c0e6638c0340c
Summary:
It's interesting to see the total number of loaded files
vs. trees when the loaded inode count is high.
Reviewed By: wez
Differential Revision: D6765874
fbshipit-source-id: 178b30184428bd5cf5e005eb475e4f5a1476c385
Summary:
I'm not sure what was wrong with the old code, but I
simplified and clarified all of the time math and now `eden debug
unload` behaves as I'd expect it should.
Reviewed By: simpkins
Differential Revision: D6764962
fbshipit-source-id: 3ed359d4ab4652e95d1538a0982c24185999351c
Summary:
bolinfest has observed `eden doctor` unmounting active mounts, so disable
that check until this is resolved.
Reviewed By: bolinfest
Differential Revision: D6768284
fbshipit-source-id: 21add1fa283f8631055019dae99819d04e179d5e
Summary:
On closer investigation, I realized that the typechecker is
complaining about a real problem here, that was apparently hidden by
the stubs. The problem is explained in
https://our.intern.facebook.com/intern/dex/qa/1596320410425583/
It's not safe to `import thrift` followed by accessing
`thrift.Thrift.TException`; it only happens to work at runtime if some
intervening code has explicitly imported `thrift.Thrift` submodule for
you.
This diff fixes the code to be safer and also make mypy happy.
Reviewed By: chadaustin
Differential Revision: D6761089
fbshipit-source-id: 5c6fdfa0bef893c4aea17ade8e7bfacaf7c0c8cc
Summary:
As noted by the TODO, enough time has passed since the migration to the new
directory layout has been available that we should feel comfortable deleting the
migration code.
Reviewed By: wez
Differential Revision: D6762457
fbshipit-source-id: 9b4a9ad1a0204878a419362c8cfbdcbb500b1aee
Summary:
If the Eden process dies or malfunctions, it's possible to end up with stale
edenfs mounts on the system. Change `eden doctor` to correct them.
Reviewed By: simpkins
Differential Revision: D6659193
fbshipit-source-id: d9fcf2e68663295e4f43b2c11fd4503a1dfac038
Summary:
Added --version to main.py, including -v and version.
It prints both installed and running version (as per Wez's suggestion), and accounts for the possibilities that eden may not be running at all, or dev version may be running.
Reviewed By: wez
Differential Revision: D6724204
fbshipit-source-id: 5085f53a00a557f759a23fe41fb57189c9ad6a7e
Summary:
D6738525 and D6731821 (https://github.com/python/typeshed/pull/1827)
broke type checking on Eden's Python. This works around removing the thrift
stubs.
Reviewed By: carljm
Differential Revision: D6745233
fbshipit-source-id: 760f7ee199698a0a570a993f16b45c833910aade
Summary:
Changed `eden clone` to check out master commit in both git and hg.
Previously, it checked out the current commit for the repo.
Reviewed By: simpkins
Differential Revision: D6663754
fbshipit-source-id: 92b185ccca5d082dc2bde9c8b191c82a2a4f06b4
Summary:
`eden list` enumerates all of the mounts in the Eden config,
whether or not the daemon is running and whether or not the mount is
currently active or not. This diff adds an (active) suffix on a mount
if the daemon is running and the mount is currently active.
Reviewed By: simpkins
Differential Revision: D6661303
fbshipit-source-id: c098e90fc9a77f16c723c707cc4da3ee3d4c5abb
Summary: eden rage crashes sometimes due to too much inode info in the info dump. Replacing per-inode info dump with the stats summary from the stats module.
Reviewed By: chadaustin
Differential Revision: D6641312
fbshipit-source-id: 16910aa21306db4e5533217bd2ffb7b37440a807
Summary:
This is a codemod to change from using @/ to // in basic cases.
- TARGETS files with lines starting with @/ (but excluding @/third-party:
- autodeps lines in source and TARGETS files ( (dep|manual)=@/ ), excluding @/third-party
- Targets in string macros
The only thing left of the old format should be @/third-party:foo:bar
drop-conflicts
Reviewed By: ttsugriy
Differential Revision: D6605465
fbshipit-source-id: ae50de2e1edb3f97c0b839d4021f38d77b7ab64c
Summary: Previously, this would cause a crash when running `eden doctor`. Now fixed.
Reviewed By: simpkins
Differential Revision: D6607211
fbshipit-source-id: a0e077beaf6b7031d57efd72b947e90884369852
Summary:
In making this change, I was hoping to remove `.encode()` calls, but the net
number is zero.
Reviewed By: chadaustin
Differential Revision: D6606997
fbshipit-source-id: ea1851d719aff0bae424f15c94a7cc48b8f516f0
Summary:
This additional debugging info should help when investigating user issues.
While here, I also fixed things up so that `eden rage` does not crash if Eden
is not running (it just prints less information and tells the user).
Reviewed By: chadaustin
Differential Revision: D6599309
fbshipit-source-id: cf5c63dcc6f4dbf224122579c3bd458730629a66
Summary:
`eden stats` used to show system memory usage which was not very
interesting (and can be gleaned from top). Instead read the contents
of /proc/self/smaps and sum the Private_Dirty fields to get a number
that more accurately reflects impact on the rest of the system.
Reviewed By: wez
Differential Revision: D6575595
fbshipit-source-id: 9badc5cd5a1b56d3ccb27edd1a2d20ee74ec34ae
Summary:
When I updated D6577031 in response to code review feedback,
I manually tested with `--qf` but at the last minute changed to the
suggested long-form `--queryfmt` without verifying it was the
appropriate name.
Reviewed By: simpkins
Differential Revision: D6579002
fbshipit-source-id: ac2a96e7340099eb83336cb11d16dd711b0d4f9f
Summary:
I forgot that `subprocess.check_output()` returns a `bytes` instead of a `str`.
This caused `eden doctor` to return false positives because the installed and
running versions of `edenfs` would be the same, but the check would fail because
it was comparing a `bytes` to a `str`.
Reviewed By: simpkins
Differential Revision: D6577031
fbshipit-source-id: 681ea22ef79604a3dfb278d713e5c68c54d8ecd5
Summary:
This reports the difference between what is returned by the `aliveSince()`
Thrift method and the current time.
Reviewed By: chadaustin
Differential Revision: D6566127
fbshipit-source-id: 449103bcb31a87e4efd780299131eb38a45a6bd7
Summary:
`eden debug buildinfo` lists the exported values from the Thrift daemon that
start with `build_`.
Reviewed By: simpkins
Differential Revision: D6565668
fbshipit-source-id: 62009d7a23211765039a5045a517113043b5d8a9
Summary:
In the initial implementation of `eden doctor`, if you had a mount point like:
/data/users/mbolin/eden-fbsource
then it looked for a pair of Watchman subscriptions for
`/data/users/mbolin/eden-fbsource` with the names:
hg-repository-watchman-subscription-primary
filewatcher-/data/users/mbolin/eden-fbsource
as a heuristic that Nuclide was being used to edit files in that Eden mount.
However, if the user is using Nuclide to edit files in a subdirectory of that
Eden mount (as determined by the Nuclide connection profile), then the
subscriptions look like this:
hg-repository-watchman-subscription-primary
filewatcher-/data/users/mbolin/eden-fbsource/<path-to-subdirectory>
In this case, `eden doctor` was returning a false positive because it thought
that the `filewatcher-/data/users/mbolin/eden-fbsource` subscription was missing
when in fact everything was just fine.
This revision changes the logic to look for a subscription to the root or one of
its subdirectories.
(Note: this ignores all push blocking failures!)
Reviewed By: simpkins
Differential Revision: D6467113
fbshipit-source-id: bfe5fafaa388405f2e59e61315c5a2084c8adc56
Summary:
This makes the `expected_output` in `stats_test.py` cleaner to declare.
(Note: this ignores all push blocking failures!)
Reviewed By: simpkins
Differential Revision: D6465282
fbshipit-source-id: 80b697c84f28a0fece77eb1050c4364b14685c0b
Summary:
Presumably for historical reasons, we still had some tests hanging around with
`from __future__ import absolute_import` in the header even though all of our
tests should be Python 3. I converted these stragglers and added a smattering of
type annotations to enforce their Python 3-ness.
(Note: this ignores all push blocking failures!)
Reviewed By: simpkins
Differential Revision: D6464805
fbshipit-source-id: 6177d7663ab428a0dbbecb287f340d770679551d
Summary: This is not material to the test, so it's distracting.
Reviewed By: wez
Differential Revision: D6462338
fbshipit-source-id: f289516c7e67891f1fed74df721fa1eb24e5f368
Summary:
`eden doctor` is a new subcommand that attempts to diagnose
issues with Eden and autofix them, as appropriate. There is also
a `--dry-run` flag that will tell the user about issues without attempting
to fix them.
Initially, `eden doctor` checks for the following:
- If Watchman has an `inotify` watcher, `eden doctor` identifies this and
will attempt to replace it with an `eden` watcher.
- If there are some active Watchman subscriptions that appear to come
from Nuclide, warn the user if the mission-critical `filewatcher-` subscription
that watches the entire repo is missing.
- If p1 in `SNAPSHOT` and `.hg/dirstate` do not match, warn the user.
The code for `eden doctor` is organized so that it should be relatively
easy to add new conditions to check for going forward. Admittedly, the
UX could be better by formatting the output (color, boldness, etc.) to
make certain information stand out, but we can improve that in
subsequent revisions.
Note that I had to do a bit of cleanup in `eden/cli/TARGETS` as part of
this revision and I created `eden/cli/test/TARGETS` so the tests have
their own build file.
Reviewed By: chadaustin
Differential Revision: D6446057
fbshipit-source-id: ae23c5996dba4f7f70f118179e5556efc29c31c3
Summary:
Previously, we used the Mercurial code `g` when faced with an `UNTRACKED_ADDED`
file conflict, but that was allowing merges to silently succeed that should not
have. This revision changes our logic to use the code `m` for merge, which
unearthed that we were not honoring the user's `update.check` setting properly.
Because we use `update.check=noconflict` internally at Facebook, we changed the
Eden integration tests to default to verifying Hg running with this setting. To
support it properly, we had to port this code from `update.py` in Mercurial to
our own `_determine_actions_for_conflicts()` function:
```
if updatecheck == 'noconflict':
for f, (m, args, msg) in actionbyfile.iteritems():
if m not in ('g', 'k', 'e', 'r', 'pr'):
msg = _("conflicting changes")
hint = _("commit or update --clean to discard changes")
raise error.Abort(msg, hint=hint)
```
However, this introduced an interesting issue where the `checkOutRevision()`
Thrift call from Hg would update the `SNAPSHOT` file on the server, but
`.hg/dirstate` would not get updated with the new parents until the update
completed on the client. With the new call to `raise error.Abort` on the client,
we could get in a state where the `SNAPSHOT` file had the hash of the commit
assuming the update succeeded, but `.hg/dirstate` reflected the reality where it
failed.
To that end, we changed `checkOutRevision()` to take a new parameter,
`checkoutMode`, which can take on one of three values: `NORMAL`, `DRY_RUN`, and
`FORCE`. Now if the user tries to do an ordinary `hg update` with
`update.check=noconflict`, we first do a `DRY_RUN` and examine the potential
conflicts. Only if the conflicts should not block the update do we proceed with
a call to `checkOutRevision()` in `NORMAL` mode.
To make this work, we had to make a number of changes to `CheckoutAction`,
`CheckoutContext`, `EdenMount`, and `TreeInode` to keep track of the
`checkoutMode` and ensure that no changes are made to the working copy when a
`DRY_RUN` is in effect.
One minor issue (for which there is a `TODO`) is that a `DRY_RUN` will not
report any `DIRECTORY_NOT_EMPTY` conflicts that may exist. As `TreeInode` is
implemented today, it is a bit messy to report this type of conflict without
modifying the working copy along the way.
Finally, any `UNTRACKED_ADDED` conflict should cause an update to
abort to match the behavior in stock Mercurial if the user has the following
config setting:
```
[commands]
update.check = noconflict
```
Though the original name for this setting was:
```
[experimental]
updatecheck = noconflict
```
Although I am on Mercurial 4.4.1, the `update.check` setting does not seem to
take effect when I run the integration tests, but the `updatecheck` setting
does, so for now, I set both in `hg_extension_test_base.py` with a `TODO` to
remove `updatecheck` once I can get `update.check` to do its job.
Reviewed By: simpkins
Differential Revision: D6366007
fbshipit-source-id: bb3ecb1270e77d59d7d9e7baa36ada61971bbc49
Summary:
Increase the eden daemon start timeout from 5 to 60 seconds.
With the 5 second timeout I sometimes see `buck test eden/...` time out in the
clone integration test which waits for eden to start. This likely takes longer
than normal when lots of other integration tests are also running in parallel.
Even when simply manually starting eden I have seen it take nearly 60 seconds
to open the RocksDB database. (I suspect RocksDB performs some extra checking
when opening a DB created by an older version of the code.) Eden requires the
RocksDB be open before it starts responding successfully to health check
requests.
Reviewed By: wez
Differential Revision: D6434357
fbshipit-source-id: 5f62ff821ecd64f7a1345e611f2299177513e411
Summary: Update the functions in cli/util.py with type annotations.
Reviewed By: bolinfest
Differential Revision: D6434356
fbshipit-source-id: 054d81427132229a390b8a133d5180be5f70bd20
Summary:
In onboarding users, we usually tell them to run `eden clone fbsource`,
but that fails because users generally have not run `eden daemon` yet.
The simplest thing is to do it for them when they run `eden clone` when
the daemon is not running.
Reviewed By: wez
Differential Revision: D6357249
fbshipit-source-id: dc112c1efe214485e3c5c8e06522d299a100d3a0
Summary:
This begins implementing the "client-side" portion of graceful takeover in
edenfs. When the --takeover flag is specified, if edenfs finds that another
edenfs process is already running it will attempt to gracefully take over its
mount points rather than exiting with an error.
This does not yet actually take over the mount points themselves--it still
sends dummy mount information during shutdown, and does not use this data
during startup. However, we do perform takeover of the eden lock file and the
thrift server socket.
Reviewed By: bolinfest
Differential Revision: D6038944
fbshipit-source-id: 42406a0559367cec79af088b4ca84c22de3f3ef3
Summary:
This fixes up a minor but sadly fatal oversight from D6314115; a call to
`get_repo_config` didn't get renamed to `find_config_for_alias`
(Note: this ignores all push blocking failures!)
Reviewed By: bolinfest
Differential Revision: D6354474
fbshipit-source-id: 13c665882419feebf0b8c0596eb3e0220ee2cf13
Summary:
It seemed unnecessary to include a `\n` in the template only to strip it.
Similarly, we should be strict when parsing the output since we expect it to be
a string of hex characters.
Reviewed By: wez
Differential Revision: D6322129
fbshipit-source-id: 61c483badfd7b68ed012310360aa582d6bdf5181
Summary:
When cloning an existing Eden mount, we should be smart and inherit its
underlying config so that we inherit properties such as its bind mounts.
Reviewed By: wez
Differential Revision: D6322002
fbshipit-source-id: 3f5ba135b12ad7dcecef6676d27495cfbf0ce97b
Summary:
Previously, a user had to define a config for a repo in a file like `~/.edenrc`
in order to create a new Eden mount via `eden clone`. In practice, the
information that is hardcoded in the config can generally be inferred from an
existing repo, so this expands `eden clone` to support both modes of operation.
Note this made it possible to finally unify the `RepoConfig` and `ClientConfig`
types. This revision removes `RepoConfig`, so I dutifully renamed every
local variable named `repo_config` to `client_config`.
Reviewed By: wez
Differential Revision: D6314115
fbshipit-source-id: 9625a5fbe35b30f76b6099180580c64435a4cf72
Summary:
As explained by the comment in the file, the alias for the config that was used
to create the client should not be necessary to mount it. This builds on top of
D6310325 to rewrite some internal functions that still relied on this alias
being present.
Reviewed By: wez
Differential Revision: D6313778
fbshipit-source-id: 867300e9ec844731993376b8957935f175a242f5
Summary:
The headline changes of this revision are:
- Changes the format of the config file from INI to TOML
(the `edenrc` file under `~/local/.eden` has been replaced
with `config.toml`). This revision includes logic for automatically
performing the migration when Eden is restarted.
- Inlines data from `/etc/eden/config.d` into the TOML file.
Historically, the `edenrc` file for a client would contain the
name of the "configuration alias" defined in a config file like
`~/.edenrc` or `/etc/eden/config.d/00-defaults`. When Eden
loaded a client, it would have to first read the `edenrc` and
then reconstitute the rest of the client configuration by
looking up the alias in the set of config files that were used to
create the client in the first place.
This changes things so that all of the data that was being
cross-referenced is now inlined in the client's config file.
This makes loading a config considerably simpler at the cost
of no longer being able to change the config for multiple clients
that were cloned from the same configuration alias in one place.
It was questionable whether being able to modify a client from
a foreign config after it was created was a safe thing to do, anyway.
Eliminating the need for a historic link to the configuration alias
will make it easier to support running `eden clone` on an arbitrary
local Hg or Git repo. So long as `eden clone` can extract enough
information from the local repo to create an appropriate config file
for the new Eden client, there is no need for a configuration alias
to exist a priori.
Since we were already changing the data in the config file, this
seemed like an appropriate time to make the switch from INI to
TOML, as this was something we wanted to do, anyway.
In testing, I discovered a discrepancy between how boost's
`boost::property_tree::ptree` and Python's `ConfigParser` handled
the following section heading:
```
[repository ZtmpZsillyZeden-clone.LIkh32]
```
Apparently `hasSection("repository ZtmpZsillyZeden-clone.LIkh32")`
in boost would fail to find this section. Because
[[https://stackoverflow.com/questions/13109506/are-hyphens-allowed-in-section-definitions-in-ini-files | there is no spec for INI]],
it is not that surprising that boost and `ConfigParser` do not 100% agree
on what they accept. Moving to TOML means we have a configuration
language with the following desirable properties:
- It has a formal spec, unlike INI. This is important because there are parsers
in a wide range of programming languages that, in theory, accept a consistent
input language.
- It is reasonable for humans to write, as it supports comments, unlike JSON.
- It supports nested structures, like maps and arrays, without going crazy
on the input language it supports, unlike YAML.
Eden now depends on the following third-party TOML parsers:
* C++ https://github.com/skystrife/cpptoml
* Python https://github.com/uiri/toml
This revision also changes the organization of `~/local/.eden` slightly. For now,
there is still a `config.json` file, but the values are no longer hashes of the realpath
of the mount. Instead, we take the basename of the realpath and use that as the
name of the directory under `~/local/.eden/clients`. If there is a naming collision, we
add the first available integral suffix. Using the basename makes it easier to
navigate the `~/local/.eden/clients` directory.
Although the `edenrc` file under `~/local/.eden/clients` has been switched from INI
to TOML, the other Eden config files (`~/.edenrc` and `/etc/eden/config.d/*`) still use
INI. Migrating those to TOML will be done in a future revision.
Note this revision allowed us to eliminate `facebook::eden::InterpolatedPropertyTree`
as well as a number of uses of boost due to the elimination of
`ClientConfig::loadConfigData()` in the C++ code. Because `ClientConfig`
no longer does interpolation, a bit of `ClientConfigTest` was deleted as part of
this revision because it is no longer relevant.
Reviewed By: wez
Differential Revision: D6310325
fbshipit-source-id: 2548149c064cdf8e78a3b3ce6fe667ff70f94f84
Summary:
This flips the sense of the unmount command; previously
we would default to destroying the mount and associated state,
but this was a bit of a massive sharp edge to our UX.
Now the default is non-destructive and you have to explicitly
pass in `--destroy` to enable destructive mode.
Reviewed By: chadaustin
Differential Revision: D6346013
fbshipit-source-id: ce612e7d8a70540d63217a97f96bc5760f3951af
Summary:
The messaging implies that something is already mounted, especially
if one is used to the error messages that are emitted by `mount`. In this
situation we're complaining that we already have a repo configured at that
path, so let's help point folks in the right direction in the error message.
Reviewed By: chadaustin
Differential Revision: D6344017
fbshipit-source-id: d48176f5806b3233e191567a840c88af101002cf
Summary:
Moving the post-clone step out of C++ makes it so that
`ClientConfig` in C++ no longer knows about `hooks` and requires
`RepoConfig` to know about `hooks`. This helps us reduce
`ClientConfig`'s dependency on `ConfigData`, as my next step
is to move the remaining information that `ClientConfig` gets from
`ConfigData` directly into the client's `edenrc` file under
`~/local/.eden`.
Reviewed By: chadaustin
Differential Revision: D6310544
fbshipit-source-id: dec7a21281ab49e0416b8872757970a4eff2d943
Summary:
This will facilitate an upcoming change where I include the `hooks` information
as part of the data returned by `get_repo_config()`
(formerly `get_repo_data()`).
Reviewed By: chadaustin
Differential Revision: D6311536
fbshipit-source-id: 2e1bc063c4a12772c9806f6880822fb15e18fc92
Summary:
This seems like something I missed during the original code review in D3554550.
In practice, there probably isn't much contention for this lock, so it's not
surprising that we haven't knowingly been bitten by this yet.
Reviewed By: chadaustin
Differential Revision: D6310326
fbshipit-source-id: c16a07aa654240be77a38c16473a4cc1e06125b6
Summary:
Unless you're going to clean up and then reraise, catching
BaseException rarely makes sense, since it swallows keyboard
interrupts.
Reviewed By: simpkins, bolinfest
Differential Revision: D6262224
fbshipit-source-id: b6b9bc963a4c75fe223c3b787ba822c3cf5da24d
Summary:
This is a major change to how we manage the dirstate in Eden's Hg extension.
Previously, the dirstate information was stored under `$EDEN_CONFIG_DIR`,
which is Eden's private storage. Any time the Mercurial extension wanted to
read or write the dirstate, it had to make a Thrift request to Eden to do so on
its behalf. The upside is that Eden could answer dirstate-related questions
independently of the Python code.
This was sufficiently different than how Mercurial's default dirstate worked
that our subclass, `eden_dirstate`, had to override quite a bit of behavior.
Failing to manage the `.hg/dirstate` file in a way similar to the way Mercurial
does has exposed some "unofficial contracts" that Mercurial has. For example,
tools like Nuclide rely on changes to the `.hg/dirstate` file as a heuristic to
determine when to invalidate its internal caches for Mercurial data.
Today, Mercurial has a well-factored `dirstatemap` abstraction that is primarily
responsible for the transactions with the dirstate's data. With this split, we can
focus on putting most of our customizations in our `eden_dirstate_map` subclass
while our `eden_dirstate` class has to override fewer methods. Because the
data is managed through the `.hg/dirstate` file, transaction logic in Mercurial that
relies on renaming/copying that file will work out-of-the-box. This change
also reduces the number of Thrift calls the Mercurial extension has to make
for operations like `hg status` or `hg add`.
In this revision, we introduce our own binary format for the `.hg/dirstate` file.
The logic to read and write this file is in `eden/py/dirstate.py`. After the first
40 bytes, which are used for the parent hashes, the next four bytes are
reserved for a version number for the file format so we can manage file format
changes going forward.
Admittedly one downside of this change is that it is a breaking change.
Ideally, users should commit all of their local changes in their existing mounts,
shutdown Eden, delete the old mounts, restart Eden, and re-clone.
In the end, this change deletes a number of Mercurial-specific code and Thrift
APIs from Eden. This is a better separation of concerns that makes Eden more
SCM-agnostic. For example, this change removes `Dirstate.cpp` and
`DirstatePersistance.cpp`, replacing them with the much simpler and more
general `Differ.cpp`. The Mercurial-specific logic from `Dirstate.cpp` that turned
a diff into an `hg status` now lives in the Mercurial extension in
`EdenThriftClient.getStatus()`, which is much more appropriate.
Note that this reverts the changes that were recently introduced in D6116105:
we now need to intercept `localrepo.localrepository.dirstate` once again.
Reviewed By: simpkins
Differential Revision: D6179950
fbshipit-source-id: 5b78904909b669c9cc606e2fe1fd118ef6eaab95
Summary:
Apparently `Dict[str, [int, int, int, int]]` is not a valid MyPy type.
Fixed to be `Dict[str, Tuple[int, int, int, int]]`.
This was introduced in a recent change (D6189543).
Reviewed By: danzimm
Differential Revision: D6241810
fbshipit-source-id: a32da67be0c555f985669e7fc2d8058f469bb371
Summary: Adds an `eden stats thrift` command that prints call counts for each Thrift call, similar to `eden stats io` printing FUSE counts.
Reviewed By: bolinfest
Differential Revision: D6189543
fbshipit-source-id: 59c29a4036687e1bf75b1c0ca2a7d311b9d1399f
