feat(foundationdb): introduce db_run #73

PierreZ · 2022-09-12T06:26:14Z

The idea is to add the db.run method, which is automatically handling Transaction retry.

This is a second try(#54 and #60), and while I know it is not perfect, it provides a great developer experience. This method is mandatory to build effective layers with the bindings, and we are currently using it at Clever Cloud.

It is also a requirement for #70 and to open-source the simulation crate that we have internally.

A new type, RetryableTransaction, has been added, which is almost like a Transaction, except that:

it does not provide cancel,
it is cloneable.

Users can still use the transact/transact_boxed methods, if they want to collaborate more with the borrow-checker, at the price of a more difficult API.

Remaining things to do if this gets merged:

convert examples to db.run,
find a way to use the borrow-checker to avoid the Arc::try_unwrap during on_error methods.

coveralls · 2022-09-12T06:43:59Z

Pull Request Test Coverage Report for Build 3143598070

29 of 58 (50.0%) changed or added relevant lines in 3 files are covered.
7 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.1%) to 74.612%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
foundationdb/src/transaction.rs	13	19	68.42%
foundationdb/src/error.rs	13	36	36.11%

Files with Coverage Reduction	New Missed Lines	%
foundationdb-gen/src/lib.rs	1	0.45%
foundationdb/src/directory/directory_partition.rs	1	81.34%
foundationdb/src/tuple/pack.rs	1	92.57%
foundationdb/src/directory/directory_layer.rs	4	94.18%

Totals
Change from base Build 3008931856:	0.1%
Covered Lines:	4323
Relevant Lines:	5794

💛 - Coveralls

codecov-commenter · 2022-09-12T06:44:55Z

Codecov Report

Base: 82.74% // Head: 82.07% // Decreases project coverage by -0.67% ⚠️

Coverage data is based on head (2171253) compared to base (22ac03c).
Patch coverage: 34.69% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
- Coverage   82.74%   82.07%   -0.68%     
==========================================
  Files          26       26              
  Lines        4956     5082     +126     
==========================================
+ Hits         4101     4171      +70     
- Misses        855      911      +56

Impacted Files	Coverage Δ
foundationdb/src/lib.rs	`100.00% <ø> (ø)`
foundationdb/src/transaction.rs	`81.77% <30.50%> (-7.59%)`	⬇️
foundationdb/src/error.rs	`63.23% <36.11%> (-30.71%)`	⬇️
foundationdb/src/database.rs	`86.59% <100.00%> (+11.32%)`	⬆️
foundationdb/src/directory/mod.rs	`94.11% <0.00%> (-0.74%)`	⬇️
foundationdb/src/tuple/pack.rs	`92.37% <0.00%> (-0.36%)`	⬇️
foundationdb/src/directory/directory_layer.rs	`93.57% <0.00%> (-0.35%)`	⬇️
foundationdb-gen/src/lib.rs	`0.44% <0.00%> (-0.01%)`	⬇️
foundationdb/src/directory/directory_partition.rs	`81.34% <0.00%> (ø)`
foundationdb/src/future.rs	`80.32% <0.00%> (+0.10%)`	⬆️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Speedy37

looks good, it's a nice tradeoff

foundationdb/src/transaction.rs

foundationdb/src/database.rs

Speedy37 · 2022-09-19T16:27:42Z

About the panic behavior.
While I agree, misusing RetryableTransaction is a bug and should, it might be good to return an Error to not crash production (maybe only in release build? or via a feature).

GAT is around the corner (next stable release) so I might give a shot at writing a more user friendly transact method but that would not change the fact that it's a &Transaction that is manipulated inside the closure.

I wonder, what was the pain of manipulating &Transaction, cause I remember having a whole object db engine (multithreaded) inside the closure and that was not a big problem. Still a few struct Foo<'trx> { } there and there, but forced correctness was what I wanted.

Speedy37 · 2022-09-19T16:33:46Z

About that point: "find a way to use the borrow-checker to avoid the Arc::try_unwrap during on_error methods."

No lifetime, no way at compile time to prove you are the only remaining owner of the transaction.

PierreZ · 2022-09-22T07:57:44Z

About the panic behavior.
While I agree, misusing RetryableTransaction is a bug and should, it might be good to return an Error to not crash production (maybe only in release build? or via a feature).

I am reworking the error from db-run to include all errors that can be thrown

PierreZ · 2022-09-22T12:14:53Z

What do you think @Speedy37 about the new error?

PierreZ · 2022-09-22T12:21:29Z

Forgot a panic, will push another commit

Speedy37 · 2022-09-22T18:46:55Z

foundationdb/src/error.rs

+            FdbBindingError::DirectoryError(directory_error) => {
+                if let DirectoryError::FdbError(e) = directory_error {
+                    Some(*e)
+                } else {
+                    None
+                }
+            }
+            FdbBindingError::HcaError(hca_error) => {
+                if let HcaError::FdbError(e) = hca_error {
+                    Some(*e)
+                } else {
+                    None
+                }
+            }


Suggested change

FdbBindingError::DirectoryError(directory_error) => {

if let DirectoryError::FdbError(e) = directory_error {

Some(*e)

} else {

None

}

}

FdbBindingError::HcaError(hca_error) => {

if let HcaError::FdbError(e) = hca_error {

Some(*e)

} else {

None

}

}

FdbBindingError::DirectoryError(DirectoryError::FdbError(e)) = > Some(*e),

FdbBindingError::HcaError(HcaError::FdbError(e)) = > Some(*e),

Speedy37 · 2022-09-22T18:49:57Z

What do you think @Speedy37 about the new error?

Looks ok to me.

Speedy37 · 2022-09-22T18:57:14Z

I just though that Arc<Transaction> (via Deref) already kinda exposed the same API as RetryableTransaction does (ie. everything that takes &self).

RetryableTransaction::commit/on_error are non public making RetryableTransaction almost the same as Arc<Transaction>

Another impl for RetryableTransaction could be:

#[derive(Clone)]
pub struct RetryableTransaction {
    inner: Arc<Transaction>,
}
impl std::ops::Deref for RetryableTransaction {
    type Target = Transaction;
    fn deref(&self) -> &Transaction {
        self.inner.deref()
    }
}
impl RetryableTransaction {
    pub(crate) fn new(t: Transaction) -> RetryableTransaction {
        RetryableTransaction { inner: Arc::new(t) }
    }
    pub(crate) fn take(self) -> Result<Transaction, FdbBindingError> {
        // checking weak references
        if Arc::weak_count(&self.inner) != 0 {
            return Err(FdbBindingError::ReferenceToTransactionKept);
        }
        Arc::try_unwrap(self.inner).map_err(|_| FdbBindingError::ReferenceToTransactionKept)
    }
    pub(crate) async fn on_error(
        self,
        err: FdbError,
    ) -> Result<Result<RetryableTransaction, FdbError>, FdbBindingError> {
        Ok(self
            .take()?
            .on_error(err)
            .await
            .map(RetryableTransaction::new))
    }
    pub(crate) async fn commit(
        self,
    ) -> Result<Result<TransactionCommitted, TransactionCommitError>, FdbBindingError> {
        self.take()?.commit().await
    }
}

PierreZ · 2022-09-22T19:37:49Z

I just though that Arc<Transaction> (via Deref) already kinda exposed the same API as RetryableTransaction does (ie. everything that takes &self).

RetryableTransaction::commit/on_error are non public making RetryableTransaction almost the same as Arc<Transaction>

Another impl for RetryableTransaction could be:

#[derive(Clone)]
pub struct RetryableTransaction {
    inner: Arc<Transaction>,
}
impl std::ops::Deref for RetryableTransaction {
    type Target = Transaction;
    fn deref(&self) -> &Transaction {
        self.inner.deref()
    }
}
impl RetryableTransaction {
    pub(crate) fn new(t: Transaction) -> RetryableTransaction {
        RetryableTransaction { inner: Arc::new(t) }
    }
    pub(crate) fn take(self) -> Result<Transaction, FdbBindingError> {
        // checking weak references
        if Arc::weak_count(&self.inner) != 0 {
            return Err(FdbBindingError::ReferenceToTransactionKept);
        }
        Arc::try_unwrap(self.inner).map_err(|_| FdbBindingError::ReferenceToTransactionKept)
    }
    pub(crate) async fn on_error(
        self,
        err: FdbError,
    ) -> Result<Result<RetryableTransaction, FdbError>, FdbBindingError> {
        Ok(self
            .take()?
            .on_error(err)
            .await
            .map(RetryableTransaction::new))
    }
    pub(crate) async fn commit(
        self,
    ) -> Result<Result<TransactionCommitted, TransactionCommitError>, FdbBindingError> {
        self.take()?.commit().await
    }
}

This is brilliant 🤯 This is making the code much more readable, thanks a lot @Speedy37 🙏

I think you deserve all the credits here, would you like to push a commit to my branch? Or may I include it in a commit?

@Speedy37

Applying the awesome suggestion from @Speedy37(Vincent Rouillé).

PierreZ · 2022-09-28T13:02:14Z

I integrated your proposal. Feel free to merge it @Speedy37 if you're ok with this.

feat(foundationdb): introduce db_run

d3cd388

PierreZ requested a review from Speedy37 September 12, 2022 06:26

PierreZ force-pushed the db_run branch from 01e49d8 to d3cd388 Compare September 12, 2022 06:29

Speedy37 approved these changes Sep 19, 2022

View reviewed changes

foundationdb/src/transaction.rs Outdated Show resolved Hide resolved

foundationdb/src/transaction.rs Outdated Show resolved Hide resolved

foundationdb/src/database.rs Outdated Show resolved Hide resolved

foundationdb/src/database.rs Outdated Show resolved Hide resolved

PierreZ added 5 commits September 22, 2022 12:51

feat(foundationdb): introduce FdbBindingError

57a34d9

chore(foundationdb): add hello_world using db.run

5704af6

chore: reformat table

d0a5bdd

chore(foundationdb): use cargo add in doc

02c9dbe

chore: clippy

a6f63c5

PierreZ added 2 commits September 22, 2022 15:32

feat(foundationdb): remove panic during RetryableTransaction::commit

c12b808

chore(ci): rename examples

2171253

Speedy37 reviewed Sep 22, 2022

View reviewed changes

Speedy37 approved these changes Sep 22, 2022

View reviewed changes

PierreZ added 2 commits September 28, 2022 14:27

chore: bump flake

df6bfe4

feat(foundationdb): rework RetryableTransaction

5bd8b78

Applying the awesome suggestion from @Speedy37(Vincent Rouillé).

Speedy37 merged commit 7bdc7ec into foundationdb-rs:main Sep 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(foundationdb): introduce db_run #73

feat(foundationdb): introduce db_run #73

PierreZ commented Sep 12, 2022 •

edited

coveralls commented Sep 12, 2022 •

edited

codecov-commenter commented Sep 12, 2022 •

edited

Speedy37 left a comment

Speedy37 commented Sep 19, 2022 •

edited

Speedy37 commented Sep 19, 2022

PierreZ commented Sep 22, 2022

PierreZ commented Sep 22, 2022

PierreZ commented Sep 22, 2022

Speedy37 Sep 22, 2022

Speedy37 commented Sep 22, 2022

Speedy37 commented Sep 22, 2022 •

edited

PierreZ commented Sep 22, 2022

PierreZ commented Sep 28, 2022

feat(foundationdb): introduce db_run #73

feat(foundationdb): introduce db_run #73

Conversation

PierreZ commented Sep 12, 2022 • edited

coveralls commented Sep 12, 2022 • edited

Pull Request Test Coverage Report for Build 3143598070

💛 - Coveralls

codecov-commenter commented Sep 12, 2022 • edited

Codecov Report

Speedy37 left a comment

Choose a reason for hiding this comment

Speedy37 commented Sep 19, 2022 • edited

Speedy37 commented Sep 19, 2022

PierreZ commented Sep 22, 2022

PierreZ commented Sep 22, 2022

PierreZ commented Sep 22, 2022

Speedy37 Sep 22, 2022

Choose a reason for hiding this comment

Speedy37 commented Sep 22, 2022

Speedy37 commented Sep 22, 2022 • edited

PierreZ commented Sep 22, 2022

PierreZ commented Sep 28, 2022

PierreZ commented Sep 12, 2022 •

edited

coveralls commented Sep 12, 2022 •

edited

codecov-commenter commented Sep 12, 2022 •

edited

Speedy37 commented Sep 19, 2022 •

edited

Speedy37 commented Sep 22, 2022 •

edited