Directory layer #217

PierreZ · 2020-12-31T12:23:01Z

This is the PR draft to resolves #27 and based on the initial work done by @garrensmith available on #186.

I wrote some basic tests, but here's a TODO list:

My rust is feeling a bit rusty, so feel free to comment 😄

EDIT: I discovered bindingTester while searching for directory validation.

coveralls · 2020-12-31T12:30:35Z

coverage: 67.934% (+0.5%) from 67.41%
when pulling be40732 on PierreZ:dev/directory
into f41fa13 on Clikengo:master.

codecov · 2020-12-31T12:30:41Z

Codecov Report

Merging #217 (a92fd9c) into master (f41fa13) will increase coverage by 2.95%.
The diff coverage is 77.46%.

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   76.58%   79.53%   +2.95%     
==========================================
  Files          18       24       +6     
  Lines        3041     4672    +1631     
==========================================
+ Hits         2329     3716    +1387     
- Misses        712      956     +244

Impacted Files	Coverage Δ
foundationdb/src/directory/error.rs	`0.00% <0.00%> (ø)`
foundationdb/src/lib.rs	`100.00% <ø> (ø)`
foundationdb/src/tuple/hca.rs	`66.11% <20.00%> (+35.88%)`	⬆️
foundationdb/src/directory/directory_partition.rs	`38.46% <38.46%> (ø)`
foundationdb/src/directory/mod.rs	`76.71% <76.71%> (ø)`
foundationdb-bindingtester/src/main.rs	`85.52% <77.85%> (-3.18%)`	⬇️
foundationdb/src/directory/node.rs	`81.35% <81.35%> (ø)`
foundationdb/src/directory/directory_layer.rs	`86.11% <86.11%> (ø)`
foundationdb/src/directory/directory_subspace.rs	`86.71% <86.71%> (ø)`
foundationdb/src/api.rs	`87.67% <100.00%> (+4.10%)`	⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f41fa13...a92fd9c. Read the comment docs.

Speedy37

I did a quick review of the code. Looks like you are making good progress 😀

Speedy37 · 2021-01-04T20:35:02Z

foundationdb/src/directory/mod.rs

+
+        let mut new_nodes = self.find_nodes(&trx, new_path.to_owned()).await?;
+        // assert that parent of the new node exists
+        if new_nodes.get(new_nodes.len() - 2).is_none() {


panic if len() < 2

You are right, I backported the code from the Go binding, and I'm wondering why we cannot move on the root-node. I'll definitely add more restrictions 😄

Speedy37 · 2021-01-04T20:47:26Z

foundationdb/src/directory/node.rs

+        for fdb_value in fdb_values {
+            let subspace = Subspace::from_bytes(fdb_value.key());
+            // stripping from subspace
+            let sub_directory: Vec<u8> = self.node_subspace.unpack(subspace.bytes())?;


You unpack bytes to bytes? hum...
What does fdb_value.key() content is?
Why not unpacking the string directly?

Cpp code does: subdir.unpack(subdirRange[i].key).getString(0)
Which in rust should be equivalent to self.node_subspace.unpack::<(String,)>(key)?

Why not unpacking the string directly?

That is a good question indeed 🤔 I will change this, thanks!

EDIT: after testing, I know why I did this: got a PackError(BadCode { found: 1, expected: Some(2) }) when trying to unpack to (String,). I think 1 stands for the const BYTES: u8 = 0x01; so I must have stored everything in bytes. The keys are generated with subspace.pack(something_in_string.bytes()), and I did not used the turbofish syntax (love the name 🤣), because I was crafting key's bytes by hand, like I'm used to do in HBase.

I will make some changes and properly create keys that can be easily deserialize. Go's binding is doing this:

tr.Set(parentNode.Sub(_SUBDIRS, path[len(path)-1]), prefix)

And Sub is accepting a TupleElement, so I should be OK regarding binding's compat 😎

Thanks a lot 👍

PierreZ · 2021-01-07T07:19:02Z

Thanks for the early review @Speedy37 👍

PierreZ · 2021-01-14T21:08:49Z

I'm having difficulties to grasp the idea of the DirectoryPartition, do you have knowledge of it, or should I ask on the forum?

PierreZ · 2021-02-03T14:44:58Z

Turns out, my first implementation and mental representation of the Directory was wrong 😛

Previous commits were not compatible with other bindings, so everything has been manually checked with the Go binding. Before reimplement delete and move, I will include the binding-checker to validate that everything is working properly.

Speedy37 · 2021-02-03T15:26:02Z

Great work :)

For the bindingtester, you can add the test to https://github.com/Clikengo/foundationdb-rs/blob/master/scripts/run_bindingtester.sh so the CI runs it.

test-name is directory I think

PierreZ · 2021-03-30T07:58:35Z

Just wanted to share that I'm finally passing a seed 🥳

Test with seed 1330929912 and concurrency 1 had 0 incorrect result(s) and 0 error(s) at API version 610
Completed directory test with random seed 1330929912 and 10 operations

TODO list updated

garrensmith · 2021-03-30T08:33:17Z

Wow awesome work.

…

________________________________ From: Pierre Zemb ***@***.***> Sent: Tuesday, March 30, 2021 9:58:50 AM To: Clikengo/foundationdb-rs ***@***.***> Cc: garren smith ***@***.***>; Mention ***@***.***> Subject: Re: [Clikengo/foundationdb-rs] [WIP] directory layer (#217) Just wanted to share that I'm finally passing a seed 🥳 Test with seed 1330929912 and concurrency 1 had 0 incorrect result(s) and 0 error(s) at API version 610 Completed directory test with random seed 1330929912 and 10 operations TODO list updated — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#217 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AABL2ASLWKSI5SNPJPZ2PL3TGGADVANCNFSM4VPL3QPA>.

PierreZ · 2021-04-13T19:38:15Z

Hey 👋

Last week, as I was digging into some issues on my implementation, I had the idea to go through Flow´s implementation(to be honest, I was only looking at the Go/Java´s code 👅) After discovering that Rust async is closer to Flow than I thought, I decided to erase everything and just backport the Flow´s implementation, which will be more maintainable in my opinion 😄

However, I stumble across two issues related to async and FdbValuesIter which is not Send. The related code can be found here:

error: future cannot be sent between threads safely
   --> foundationdb/src/directory/directory_layer.rs:665:5
    |
665 |     #[async_recursion]
    |     ^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
    |
    = help: within `FdbValuesIter`, the trait `std::marker::Send` is not implemented for `Rc<FdbFutureHandle>`
note: future is not `Send` as this value is used across an await
   --> foundationdb/src/directory/directory_layer.rs:688:17
    |
679 |             let range: Arc<FairMutex<FdbValuesIter>> = Arc::new(FairMutex::new(range.into_iter()));
    |                 ----- has type `Arc<parking_lot::lock_api::Mutex<RawFairMutex, FdbValuesIter>>` which is not `Send`
...
688 |                 self.remove_recursive(trx, sub_node).await?;
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ await occurs here, with `range` maybe used later
...
694 |         }
    |         - `range` is later dropped here
    = note: required for the cast to the object type `dyn futures::Future<Output = std::result::Result<(), DirectoryError>> + std::marker::Send`
    = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: future cannot be sent between threads safely
   --> foundationdb/src/directory/directory_layer.rs:665:5
    |
665 |     #[async_recursion]
    |     ^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
    |
    = help: within `FdbValuesIter`, the trait `std::marker::Send` is not implemented for `*const keyvalue`
note: future is not `Send` as this value is used across an await
   --> foundationdb/src/directory/directory_layer.rs:688:17
    |
679 |             let range: Arc<FairMutex<FdbValuesIter>> = Arc::new(FairMutex::new(range.into_iter()));
    |                 ----- has type `Arc<parking_lot::lock_api::Mutex<RawFairMutex, FdbValuesIter>>` which is not `Send`
...
688 |                 self.remove_recursive(trx, sub_node).await?;
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ await occurs here, with `range` maybe used later
...
694 |         }
    |         - `range` is later dropped here
    = note: required for the cast to the object type `dyn futures::Future<Output = std::result::Result<(), DirectoryError>> + std::marker::Send`
    = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

FdbValuesIter is defined like this:

/// An iterator of keyvalues owned by a foundationDB future
pub struct FdbValuesIter {
    f: Rc<FdbFutureHandle>,
    keyvalues: *const fdb_sys::FDBKeyValue,
    len: i32,
    pos: i32,
}

and the first two fields are the one not Send able. Could you help me fix it? I tried the Arc<Mutex<>> technique, without luck. Thanks in advance 😃

Speedy37 · 2021-04-13T20:10:21Z

Hi,

Nice work 👏

About the Send issue, I see 2 ways to try to fix it :

Very simple: maybe FdbValuesIter and FdbValue should contains an Arc<FdbFutureHandle>. Then you are allowed to add unsafe impl Send for FdbValuesIter {} and unsafe impl Send for FdbValue {}. The lifetime of *const fdb_sys::FDBKeyValue is the same as the handle.
Return a non Send future: don't use #[async_recursive attribute] and write the workaround yourself (wrapper method that box the future to have a finite stack size).

I would personally go for 1 I think.

PierreZ · 2021-04-14T08:04:59Z

Hi,

Nice work clap

About the Send issue, I see 2 ways to try to fix it :

1. Very simple: maybe `FdbValuesIter` and `FdbValue` should contains an `Arc<FdbFutureHandle>`. Then you are allowed to add `unsafe impl Send for FdbValuesIter {}` and `unsafe impl Send for FdbValue {}`. The lifetime of `*const fdb_sys::FDBKeyValue` is the same as the handle.

2. Return a non Send future: don't use `#[async_recursive attribute]` and write the workaround yourself (wrapper method that box the future to have a finite stack size).

I would personally go for 1 I think.

Thanks a lot @Speedy37, option 1 worked perfectly 🚀

PierreZ · 2021-04-16T14:01:35Z

I finished my first iteration of backporting Flow´s code, now I´m working on making tests passes 😄

PierreZ · 2021-05-08T13:04:19Z

Most of my tests are passing on most seeds, except when I'm using the DirectoryPartitions. I'm struggling to find the problem here for now, keep digging

PierreZ · 2021-06-06T17:02:14Z

Still digging 😞 I am experiencing quite some difficulties fixing the last bugs, as fixing one seed is creating new bugs in seeds that used to pass 👅

Most of my mistakes seems to be related to:

layer (sometimes I cannot find the layer under the right key). I tried removing every clear from my implementation, without luck
DirectoryPartition (exists and list are failing most of the time), due to path's manipulation between the directory Partition/Subspace/Layer. I'm backporting this class from Flow/C++ but I must admit my C++ is even more rusty than my rust 😃 I'm not sure if the DirectoryPartition is used a lot, but we cannot disable it from the bindingtester.

I could use some help 😇 If someone is willing to help me, I've marked some faulty seeds here.

PierreZ · 2021-06-21T07:13:36Z

Unblocked, thanks to @Geal 🎉

Got a dedicated computer searching for faulty seed with 10k ops. I will fix them and cleanup the code 😄

Provides structures for managing directories in FoundationDB. The FoundationDB API provides directories as a tool for managing related Subspaces. Directories are a recommended approach for administering applications. Each application should create or open at least one directory to manage its subspaces. For general guidance on directory usage, see the discussion in the Developer Guide. Directories are identified by hierarchical paths analogous to the paths in a Unix-like file system. A path is represented as a List of strings. Each directory has an associated subspace used to store its content. The layer maps each path to a short prefix used for the corresponding subspace. In effect, directories provide a level of indirection for access to subspaces.

PierreZ · 2021-06-24T11:10:58Z

The bindingtester have been running for 24h+ with 10k ops without finding a faulty seed 😄

I squashed the commits, ready to be reviewed 🎉

As mentionned before, I tried to be as close as possible from the Flow code, but it can be changed.

pierrebelzile · 2022-06-06T19:00:26Z

I was looking for this functionality today. I see that was a large amount of work. Has been in review for nearly a year. What would be next step for the merge?

PierreZ · 2022-06-07T06:15:00Z

I was looking for this functionality today. I see that was a large amount of work. Has been in review for nearly a year. What would be next step for the merge?

Hi @pierrebelzile 👋

Please note that the development of the crate has moved to the dedicated Github org. The fork is including the directory feature, and several others. Support for FDB 7.1 is also on its way.

More info can be found here on why we had to hard fork can be found here.

pierrebelzile · 2022-06-07T14:28:52Z

Thanks. Can't believe I missed that. Great news and thanks for the work! :)

garrensmith added 2 commits December 31, 2020 15:43

setup

250b8fd

initial basic work

97e9d18

PierreZ force-pushed the dev/directory branch 2 times, most recently from ce614fe to 17129a6 Compare January 3, 2021 14:33

Speedy37 reviewed Jan 4, 2021

View reviewed changes

PierreZ force-pushed the dev/directory branch 2 times, most recently from 920918a to 6948175 Compare January 10, 2021 19:18

PierreZ force-pushed the dev/directory branch 2 times, most recently from ef0e236 to 852f993 Compare February 13, 2021 12:56

PierreZ force-pushed the dev/directory branch 2 times, most recently from 57afd9a to 711edff Compare March 30, 2021 07:55

PierreZ force-pushed the dev/directory branch 4 times, most recently from 0e8281a to a9e07cd Compare April 16, 2021 12:54

PierreZ force-pushed the dev/directory branch from a9e07cd to b2bd0b5 Compare April 20, 2021 20:30

PierreZ force-pushed the dev/directory branch from b2bd0b5 to ee80e02 Compare May 8, 2021 13:02

PierreZ force-pushed the dev/directory branch from 9ba2124 to fd9afee Compare June 24, 2021 11:06

PierreZ marked this pull request as ready for review June 24, 2021 11:11

PierreZ changed the title ~~[WIP] directory layer~~ Directory layer Jun 24, 2021

cargo clippy

a92fd9c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directory layer #217

Directory layer #217

PierreZ commented Dec 31, 2020 •

edited

Loading

coveralls commented Dec 31, 2020 •

edited

Loading

codecov bot commented Dec 31, 2020 •

edited

Loading

Speedy37 left a comment

Speedy37 Jan 4, 2021

PierreZ Jan 7, 2021 •

edited

Loading

Speedy37 Jan 4, 2021

PierreZ Jan 7, 2021 •

edited

Loading

PierreZ commented Jan 7, 2021

PierreZ commented Jan 14, 2021

PierreZ commented Feb 3, 2021

Speedy37 commented Feb 3, 2021 •

edited

Loading

PierreZ commented Mar 30, 2021

garrensmith commented Mar 30, 2021 via email

PierreZ commented Apr 13, 2021

Speedy37 commented Apr 13, 2021 •

edited

Loading

PierreZ commented Apr 14, 2021

PierreZ commented Apr 16, 2021

PierreZ commented May 8, 2021

PierreZ commented Jun 6, 2021

PierreZ commented Jun 21, 2021

PierreZ commented Jun 24, 2021

pierrebelzile commented Jun 6, 2022

PierreZ commented Jun 7, 2022

pierrebelzile commented Jun 7, 2022

Directory layer #217

Are you sure you want to change the base?

Directory layer #217

Conversation

PierreZ commented Dec 31, 2020 • edited Loading

coveralls commented Dec 31, 2020 • edited Loading

codecov bot commented Dec 31, 2020 • edited Loading

Codecov Report

Speedy37 left a comment

Choose a reason for hiding this comment

Speedy37 Jan 4, 2021

Choose a reason for hiding this comment

PierreZ Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

Speedy37 Jan 4, 2021

Choose a reason for hiding this comment

PierreZ Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

PierreZ commented Jan 7, 2021

PierreZ commented Jan 14, 2021

PierreZ commented Feb 3, 2021

Speedy37 commented Feb 3, 2021 • edited Loading

PierreZ commented Mar 30, 2021

garrensmith commented Mar 30, 2021 via email

PierreZ commented Apr 13, 2021

Speedy37 commented Apr 13, 2021 • edited Loading

PierreZ commented Apr 14, 2021

PierreZ commented Apr 16, 2021

PierreZ commented May 8, 2021

PierreZ commented Jun 6, 2021

PierreZ commented Jun 21, 2021

PierreZ commented Jun 24, 2021

pierrebelzile commented Jun 6, 2022

PierreZ commented Jun 7, 2022

pierrebelzile commented Jun 7, 2022

PierreZ commented Dec 31, 2020 •

edited

Loading

coveralls commented Dec 31, 2020 •

edited

Loading

codecov bot commented Dec 31, 2020 •

edited

Loading

PierreZ Jan 7, 2021 •

edited

Loading

PierreZ Jan 7, 2021 •

edited

Loading

Speedy37 commented Feb 3, 2021 •

edited

Loading

Speedy37 commented Apr 13, 2021 •

edited

Loading