Summary:
The Rust Thrift implementation presently compiles to 4 crates:
```lang=mermaid
flowchart TD;
foo --> foo-clients;
foo --> foo-types;
foo --> foo-services;
foo-clients --> foo-types;
foo-services --> foo-types;
```
Sadly, the one with the best name is the most useless. That top-level foo crate just contains the following:
```
pub use ::foo_types as types;
pub use ::foo_clients as client;
pub use ::foo_services as server;
pub use ::foo_types::consts;
pub use ::foo_types::errors;
pub use ::foo_types::services;
pub use ::foo_clients::mock;
pub use self::consts::*;
pub use self::errors::*;
pub use self::types::*;
```
Furthermore, it is extremely uncommon for anything to want to be both a client and a server of the *same* Thrift interface. It arises occasionally for proxies, but otherwise you are either implementing the server for a particular service, or accessing it as a client, not both.
I am interested in eliminating the useless crate, and renaming the types subcrate to the nice name.
```lang=mermaid
flowchart TD;
foo-clients --> foo;
foo-services --> foo;
```
### Implementation plan
1. This diff
2. Migrate everything that uses `:foo-rust` and needs a client to `:foo-rust-clients`, and everything that needs a server to `:foo-rust-services` -- after this point, everything still depending on `:foo-rust` only uses types
3. In the macros, atomically delete `:foo-rust` and rename `:foo-rust-types` into its place (this will be a small diff)
Reviewed By: zertosh, shayne-fletcher
Differential Revision: D53302058
fbshipit-source-id: 8d28c90f3a13e71e0cedd3ce3c1cb6fe40cffe27
Summary:
# Before:
If a Thrift library A depends on Thrift library B, all of the dependencies on B are bottlenecked through B's main library. That means building A's clients requires building B's servers, and building A's servers requires building B's clients, neither of which should be necessary.
```lang=mermaid
flowchart TD;
A --> A-clients;
A --> A-types;
A --> A-services;
A-clients --> A-types;
A-services --> A-types;
B --> B-clients;
B --> B-types;
B --> B-services;
B-clients --> B-types;
B-services --> B-types;
A --> B;
A-clients --> B;
A-services --> B;
A-types --> B;
```
# After:
A tidy lattice. Projects that implement a Thrift service can build only the servers for A and B, without also building all their clients. Projects that need Thrift clients for various services can build just the clients without also building a bunch of unused infrastructure for writing a server implementation.
```lang=mermaid
flowchart TD;
A --> A-clients;
A --> A-types;
A --> A-services;
A-clients --> A-types;
A-services --> A-types;
B --> B-clients;
B --> B-types;
B --> B-services;
B-clients --> B-types;
B-services --> B-types;
A --> B;
A-clients --> B-clients;
A-services --> B-services;
A-types --> B-types;
```
Reviewed By: shayne-fletcher
Differential Revision: D53246158
fbshipit-source-id: 37cc5cb111c39c567c69e8fb2eaf23fd940082b3
Summary: `folly::StringKeyedUnorderedMap` used to be a complicated class, but is now a simple typedef to `F14NodeMap<std::string, Mapped>`. Just use the latter.
Reviewed By: bcardosolopes, xavierd
Differential Revision: D53232021
fbshipit-source-id: 4b545d1c912dae2bbf89e31354f831d818c85167
Summary:
They still haven't released 0.5.0, but made some improvements on the path
there. This brings us back to their newest commit
To keep backwards compatibility (and fix the tests that break because of broken bc), this needs to be landed together with D53111463.
Reviewed By: dtolnay
Differential Revision: D53074256
fbshipit-source-id: 124badef3c923acda758e75f1a4437d8826d5f7c
Summary:
Update libbpf-sys to version 1.3.0+v1.3.0. This release includes libbpf in
version v1.3.0, which is what we were waiting on. Because this release contains
some incompatibilities, we also have to update libbpf-rs to the most recent
snapshot. A new libbpf-rs release will come once sufficient features have
accumulated.
Reviewed By: shayne-fletcher
Differential Revision: D53102023
fbshipit-source-id: 47b74d38b04237d4d5ba1f93fe19e83b9d2b6125
Summary:
## Problem
Having an "automatic remediation" for full storage is misleading:
1) Our remediation may not actually solve the full disk space issue
2) Our remediation doesn't actually work since the `apfs.util` command we run requires `sudo` to correctly purge all purgeable space.
## Solution
Instead of claiming that we automatically remediated the issue, let's inform the user of the potential problem, tell them how they can check if that's the *actual* problem, and then give them a remediation they can run to fix the problem. While this is strictly worse than automatically running the remediation for them, it's much better than running a remediation that doesn't actually work and claiming it fixed the problem.
Reviewed By: genevievehelsel
Differential Revision: D53200901
fbshipit-source-id: 23dc142b4c0d236d8b31bba85d0a8f98fe76aeba
Summary:
# Summary:
Keeping a vector of config options, manually deserializing/serializing them, and then using string-comparisions to check the type seems like a bad long-term trend. Let's move towards using enums for these types of things (like I introduced in D53146609).
# Solution:
This diff enum-izes (enum-erizes) the InodeCatalogType that we parse from the CheckoutConfigs.
Reviewed By: jdelliot
Differential Revision: D53151826
fbshipit-source-id: b646f92f5a308a46d37d43758535cc5dfdd58164
Summary: See title. I liked this message better than the old one.
Reviewed By: jdelliot
Differential Revision: D53151834
fbshipit-source-id: 05dad98d79572972675653ec873f497ae8ab9ee3
Summary:
# Summary:
Keeping a vector of config options, manually deserializing/serializing them, and then using string-comparisions to check the type seems like a bad long-term trend. Let's move towards using enums for these types of things (like I introduced in D53146609).
# Solution:
This diff enum-izes (enum-erizes) the MountProtocols that we parse from the CheckoutConfigs.
Reviewed By: jdelliot
Differential Revision: D53151833
fbshipit-source-id: b2e71e6a8ba451fcf46e7d2119b6be3b3ce0f4c4
Summary:
# Problem:
FilteredFS repos write FilteredRootIds to the Snapshot file so that they can track the current working copy parent, active FilterID, and parse those two pieces of information as needed.
This means that the current getSnapshot logic naively returns FilteredRootIds instead of the expected commit hashes as expected.
# Solution:
We can modify getSnapshot to properly parse the FilteredRootIds and return only the commit hash to the caller.
Reviewed By: jdelliot
Differential Revision: D53146665
fbshipit-source-id: c38980f00b977f525fba654e146e88f8e1693c43
Summary:
# Problem:
The next diff implements correct getSnapshot() logic which requires doing a comparison of the repo_type config field. Instead of comparing raw strings, we should compare enums.
# Solution:
We can parse the repo_type CheckoutConfig option as an enum to avoid string comparison for determining repo types.
This requires implementing to/from string implementations for the new RepositoryType enum. We can use the strum crate to do this for us.
We can then use the to/from string implementations to deserialize (and serialize) CheckoutConfigs.
Reviewed By: kmancini
Differential Revision: D53146609
fbshipit-source-id: 29cc702695381021b210f85f83855b84dc11534e
Summary:
I discovered a case where `eden du` passes in a FilteredFS commit hash into getScmStatus (v1). This causes `eden du` to crash.
This bug stems from the Rust version of get_snapshot() not parsing out the filter information for FilteredFS Snapshot files: https://www.internalfb.com/code/fbsource/[8496d7622d579ebf9253522ec56b35a1353c25ad]/fbcode/eden/fs/cli_rs/edenfs-client/src/checkout.rs?lines=557-612
We correctly parse out the Filter info in the Python CLI (since we have varint utils implemented), but we don't in the Rust CLI because we don't have varint encode/decode functions available.
This diff adds the utils to the Rust CLI so I can correctly parse out filter info and fix `eden du`
Reviewed By: kmancini
Differential Revision: D52928980
fbshipit-source-id: 3258acacde639adaf66e53e0c49578fbb0b1e782
Summary:
X-link: https://github.com/facebookincubator/velox/pull/8558
The attribute `[[fallthrough]]` is standard C++17. We no longer need to wrap it.
Reviewed By: Orvid
Differential Revision: D53034074
fbshipit-source-id: 19b1e8314e70f3e08882e76b93f7ded4df7ab9f2
Summary: Use the commit with the fix while we wait for a new release.
Differential Revision: D53127691
fbshipit-source-id: 6128ea1ffe00d7e8d8081d7ea5093e8bb813c04f
Summary:
make it possible to call `shlex::try_join` and remove the temporary `#[allow(deprecated)]` from buck2_wrapper_common.
buck2/Cargo.toml is restored to the state it had in b9dddef8e77c334a30071810e4ef851e8ad945bb before the workaround applied in D53087714 (which implies at this time shlex-1.3.0).
Reviewed By: ndmitchell, samkevich
Differential Revision: D53001954
fbshipit-source-id: 4267b80b5884b1a1e75c938af641e3f2656bf9a7
Summary:
Users are seeing the 10K write notification pretty frequently from
`eden doctor`, even when things are working ok.
From what I understand 10K was kinda an arbitrary number that seemed reasonable
to start with when we introduced this warning.
I'm gonna attempt to pick a better threshold.
We know that we see problems at 100K - George was able to repro slowness when
things got this high.
Looking at the percentiles p99.5 is less than the 10K in 10 minute limit.
but the p99.9 is pretty much the max - >100sK in 10 minute. We don't have
global numbers which would probaly yeild more helpful percentiles. So I looked
at the top few users.
There is only a handful of users over 100K/10 min, but generally the chart is
much dense under 50K/10min. 50K probably better to warn a reasonable amount.
Let's try 50K per 10 minutes.
Differential Revision: D53066716
fbshipit-source-id: 5f99438f83db51f6d093ecb0124616ffb05fdf21
Summary:
The nested-name lock-holder `folly::SharedMutex::ReadHolder` accepts `folly::SharedMutex const&`, i.e., as ref-to-const, and it internally does a `const_cast` to remove the `const`. This internal `const_cast` is necessary because members `folly::SharedMutex::lock_shared` and related member functions are not `const`-qualified.
This lock-holder interface is a convenience since a shared-lock on a shared-mutex instance are commonly acquired in `const`-qualified class member functions, where the shared-mutex instance is a class data member and access to it from a `const`-qualified member function implicitly has only `const`-access to the shared-mutex instance.
But `std::shared_lock` does not have this convenience. It is the canonical lock-holder for shared locks and we wish to migrate all uses of `folly::SharedMutex::ReadHolder` to `std::shared_lock`. So we must mark relevant shared-mutex instances with `mutable`.
As a small shortcut, we search for and mark all class data members of type `folly::SharedMutex` as `mutable`.
Reviewed By: brekhus, mogeb
Differential Revision: D52921279
fbshipit-source-id: 99f0fc750a975d181b35c1320c964183f93caefd
Summary:
This version is working, see integration test in following diff
It can correctly do partial glob matching so that directories won't be filtered out prematurely
However, this version still needs improvement:
1. I can probably do similar thing to Michael's promise in HgSparseFilter
2. pass u8 to rust instead of a vec of string
3. check on windows
Reviewed By: kmancini
Differential Revision: D50427596
fbshipit-source-id: b95f1c6333978ee79ad1ba1f28cac9cd64441fc5
Summary:
There is a typo in this config name. The config we use in the CLI that
determines the state of the rollout is `experimental:windows-symlinks` instead
of `experimental:windows-symlinks-enabled`. This causes warning for the users.
Confirmed we don't use the `experimental:windows-symlinks-enabled` value
anywhere. and checked that we don't use `EdenConfig.windowsSymlinksEnabled`
value anywhere in the daemon.
I'm changing the default value to false too because the cli was asking the
daemon for the config and defaulting to false when the config was not set.
This ensures that symlinks stay disable.
Reviewed By: sggutier
Differential Revision: D52891836
fbshipit-source-id: cd25f16a73a6d2e2202dc16a4bfa8e052c8240b7
Summary:
This config is only used in the CLI so the daemon doesn't know how to parse it
and shows a warning. Warning is noisy and makes users think there is something
wrong. Getting rid of the warning.
Reviewed By: genevievehelsel
Differential Revision: D52889490
fbshipit-source-id: f2525580c86c0cf78b71869eb27396ac2cdc282b
Summary: After removing HgImporter there are still a few places where the name persists. Let's fix that. Renaming the thread pool factory to SaplingRetryThreadFactory and the thread pool member to retryThreadPool_. Also updated some docs to reflect and converted from Mercurial to Sapling.
Reviewed By: mshroyer, genevievehelsel
Differential Revision: D52571338
fbshipit-source-id: 4738c522d642d7a8aa66a368440e54b144a241b3
Summary: With the removal of all HgImporter code, we no longer need to differentiate fetch_miss sources. If in the future we have other data stores that fetch from we can add this back. For now, it is just taking up extra space in our scuba table.
Reviewed By: genevievehelsel
Differential Revision: D52404111
fbshipit-source-id: 792d424d5b0459cdd0e47dad45d4bfb1298a1b3d
Summary: HgImporter is no longer in use in our codebase. We can now safely remove it.
Reviewed By: genevievehelsel
Differential Revision: D51911685
fbshipit-source-id: 239d70fb89a5db364606cb0140bc585fc5128324
Summary: We had already removed fetching trees and blobs. Now, we need to remove the last bits of HgImporter.
Reviewed By: genevievehelsel
Differential Revision: D51911205
fbshipit-source-id: 07bdaf14a62f7af951eafa7e8720f681914ba3ab
Summary: HgImporter is being removed - this change removes its support for fetching trees.
Reviewed By: kmancini
Differential Revision: D51872978
fbshipit-source-id: 5b373565c6b29ecc3dc7988695b6e4f62d9a04fb
Summary: HgImporter is being removed - this change removes its support for importing blobs.
Reviewed By: kmancini
Differential Revision: D51832464
fbshipit-source-id: 504a307e3c5cb66366f50a43362667e709512552
Summary: The comment was somewhat unclear. Expand on the soft vs hard choice and add a man page that future devs can reference to understand each option.
Reviewed By: genevievehelsel
Differential Revision: D52710081
fbshipit-source-id: 53bc2eae3799e775a8bbc5dad9bb99ca6e713632
Summary: Some users are reporting seeing symlinks enabled even with the config for explicitly disabling symlinks set to false. This was caused by the Rust CLI always defaulting this to true, regardless of anything else (which this diff fixes)
Reviewed By: kmancini
Differential Revision: D52665462
fbshipit-source-id: 3e4e60c2a7d95c863abdc3d2064a67a808fc5c45
Summary:
Adds a generalized check for network connectivity that sees whether we can
establish a TCP connection to the configured EdenAPI endpoint (not just
mononoke.edge.x2p.facebook.net).
In addition to determining whether name resolution succeeds, this updated
check:
- Confirms that we have a configured route to the endpoint. In particular, if
we're configured to use an IPv6-only endpoint but don't have an IPv6 default
route configured, this check will fail.
- Verifies that we can actually establish a TCP connection to the endpoint (so
we know we aren't firewalled off, for example).
Reviewed By: kmancini
Differential Revision: D52631801
fbshipit-source-id: 3137cc602ac6cfd577da6f467567ac907ae236b6
Summary:
Help noobs such as myself with occasional corruptions on Windows.
For me, I was getting this error when running `eden doctor`:
```
<snip>
File "C:\tools\eden\libexec\edenfsctl.real.par\eden\fs\cli\config.py", line 1320, in _read_config
File "C:\tools\eden\libexec\edenfsctl.real.par\eden\fs\cli\config.py", line 1916, in load_toml_config
eden.fs.cli.config.FileError: toml config is either missing or corrupted : Reserved escape sequence used (line 18 column 1 char 386)
```
Which is completely useless because it doesn't say which file fail to parse.
After this change:
```
<snip>
File "C:\tools\eden\libexec\edenfsctl.real.par\eden\fs\cli\config.py", line 1320, in _read_config
File "C:\tools\eden\libexec\edenfsctl.real.par\eden\fs\cli\config.py", line 1916, in load_toml_config
eden.fs.cli.config.FileError: toml config file C:\Users\liorda\.eden\clients\fbsource\config.toml is either missing or corrupted: Reserved escape sequence used (line 18 column 1 char 386)
```
The error, btw, was in this line:
```
active = [ "c:\open\fbsource\arvr\projects\xpd\projects\smials\*",]
```
Created from CodeHub with https://fburl.com/edit-in-codehub
Reviewed By: xavierd
Differential Revision: D52531754
fbshipit-source-id: 298422ff283e2bd2688b4dcb9613751a6705c48a
Summary: A newer version of colored adds support for true color terminal text coloring. The changelog only indicates "Alter Color interface to return Cow<'static, str>" besides the true color support as a "change" in the changelog and I'm not expecting people to really use this. If they do, I'll see it in Sandcastle?
Reviewed By: dtolnay
Differential Revision: D52391497
fbshipit-source-id: c08fffc171f7d5f9b75f96617aa673867ab45312
Summary: Doesn't hurt to add some more tests, and I have a change I want to add a test for, so sets up some initial tests for the semi-complex functions that read from disk from `edenfs_config_util.py`
Reviewed By: MichaelCuevas
Differential Revision: D52383458
fbshipit-source-id: a512b75219740da8bef3d009c9d8a14b84b80083
Summary: Nit to have both clis show the same global flag descriptions
Reviewed By: MichaelCuevas
Differential Revision: D52422910
fbshipit-source-id: 47027f0d4aa12aa22f45bef0d7df33bed2d4732a
Summary: For readability/clearness, group the global CLI options separately from subcommand options in `--help`
Reviewed By: MichaelCuevas
Differential Revision: D52422889
fbshipit-source-id: 13b8d9361f4d5cc0b66713053ec0208c1ed8172e
Summary:
`edenfsctl_config_manager` was previously causing cmd window to flash on Windows: https://fb.workplace.com/groups/edenfswindows/posts/1414741589441715
I attempted to use a subprocess like how we do for `edenfsctl start --if-necessary` and `CoCo`, but it was still popping up a window. I missed this instance of `subprocess`, which looks like it still causes the command window to flash.
Testing the config manager with this change gets rid of the command window flashing.
Reviewed By: MichaelCuevas
Differential Revision: D52371460
fbshipit-source-id: a1e791f122d44a7a00063b5ab33949d25fb7055a
Summary:
Pull Request resolved: https://github.com/facebook/sapling/pull/794
`-Wextra-semi` or `-Wextra-semi-stmt`
If the code compiles, this is safe to land.
Reviewed By: palmje
Differential Revision: D51995090
fbshipit-source-id: 4267224b633ab3af176f11752d33b74b59bab5b0
Summary: renderObjectID should probably show the actual FilteredObjectID even if it breaks tests. I'll update the broken tests in later diffs. For now, I'll skip them.
Reviewed By: kmancini
Differential Revision: D52302469
fbshipit-source-id: 6607c7bad95fd4e66b0701a88bfe58c3e4ad47b7
Summary:
I initially only added Filter parameters to thrift endpoints that were used by Mercurial. This is because Mercurial is the only tool that's allowed to change filters and therefore change the view of the repo.
However, this causes issues for other Thrift endpoints that accept Commit hashes or RootIds. Eden expects these passed in hashes/IDs to be FilteredBackingStore friendly, and they are usually not (instead they usually are only valid for the underlying BackingStore. In most cases these are Mercurial commit hashes).
To fix this, we assume that any RootIds/hashes we receive from non-Filtered endpoints should be transformed into a FilteredRootId/ObjectID and use whatever the last used Filter was. This allows us to pass valid IDs into the FilteredBackingStore without requiring the user to supply the filter they want to use.
In the future, we can add filter params to all thrift endpoints to allow all callers to use arbitrary filters, but we should carefully think through the implications of allowing any caller to apply any filter to the repo.
Note: another option was to apply the null filter to any IDs that were passed in by these thrift endpoints. That feels wrong to me, since most callers would want to match and get results based on the view of the repo that the user sees. Since the user will see the repo with whatever filter was last applied via checkoutRevision(), it makes sense to apply this filter to the passed in IDs.
Reviewed By: kmancini
Differential Revision: D51926554
fbshipit-source-id: b4f2190904096ca68a2679ac3c7b2d7f449ffeda
Summary: The intent of RenderObjectId and RenderRootId is that it returns a string that's "human readable" or sane for a caller to examine. Returning a FilteredObjectId or FilteredRootId isn't really the correct thing to do here (callers don't care about the internal details of the FilteredBackingStore). Instead, we should render the Object/Root Id from the underlying BackingStore since that's what most callers would expect.
Reviewed By: kmancini
Differential Revision: D51926553
fbshipit-source-id: 1e23c2b9501a75d0b817e60b6f698e3b821f2551
Summary:
Two lifetime issues:
1) We need to ensure the ParentCommit structure stays alive long enough to parse the VarInt and filterID from it.
2) The prefetchBlobs method expected the caller to preserve the lifetime of the ObjectIDs that were passed in. Since FilteredBackingStore::prefetchBlobs() transforms FilteredObjectIDs into regular ObjectIDs and then passes those into the underlying BackingStore, it needs to ensure that the regular ObjectIDs it created stay alive long enough to be used by the underlying BackingStore. This can be guaranteed with an ensure().
Note: I'd love to use a UniquePtr to guarantee the lifetime of the newly created ObjectIds, but that would require changing the signature of prefetchBlobs.
Reviewed By: kmancini
Differential Revision: D51926559
fbshipit-source-id: 45b52079f8081ee76368ae4167c29c11f5c97908
Summary:
In some cases, it may be necessary to determine what filter was last applied to the repo. This would be important for filter-unaware tools that want to issue thrift queries but don't have filter context. By default, these tools probably want to use the last applied filter, and therefore we need a way to figure out what the last applied filter was.
Luckily Eden already stores this information (indirectly) in the Snapshot file. The Snapshot file contains two things: the working copy parent RootID and the last checked out commit RootID. We can derive the last applied filter by looking at either one of these, since all FilteredBackingStore RootIDs contain filter information.
Reviewed By: kmancini
Differential Revision: D51926555
fbshipit-source-id: 818548f760046364a9abe758094d8234defc51f7
Summary: We can now share parsing logic in the CheckoutConfig layer when we're parsing the last RootID from the SNAPSHOT file.
Reviewed By: kmancini
Differential Revision: D52185706
fbshipit-source-id: f9aa8a9d4b7d349917b80da38b6eb5ec368ba227
Summary:
When running `eden info`, users probably want the commit hash WITHOUT filter information attached to it. This diff removes filter information (and therefore fixes the `eden info` test).
I can consider adding a "Filter" field to Eden info. However, this is low priority right now since I want to get other tests fixed first.
Reviewed By: kmancini
Differential Revision: D51926557
fbshipit-source-id: 98480a252816384681008e9059b479e48639f9a2
Summary:
When using FilteredBackingStores, Eden now stores FilteredRootIds in the Snapshot file. FilteredRootIds consist of the original underlying BackingStore's RootId, the length of the FilteredRootId (in varint format), and the filter that was applied to the repo during the last checkout.
To properly parse out the original underlying RootId (in most cases a Mercurial commit hash), we need to parse the varint that describes the length of the underlying RootId and parse the Snapshot file accordingly.
This diff introduces the utils that will be used to do that varint parsing logic.
Reviewed By: kmancini
Differential Revision: D51926558
fbshipit-source-id: f35ba4af4cf59e593fe86b4411d7f94cd722946d
Summary: Readdir tests previously failed because the render method tried to pass a FilteredObjectID into the underlying BackingStore's renderObjectId() method. This diff parses the underlying ObjectID out and passes that into the underlying BackingStore instead of passing in an uninterpretable FilteredObjectId.
Reviewed By: kmancini
Differential Revision: D51829836
fbshipit-source-id: 865cbf8f5e5f73bce77fd8ab32a315c307a741e9
