Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for FoundationDB 7.1 #61

Merged
merged 8 commits into from
Jul 4, 2022
Merged

Support for FoundationDB 7.1 #61

merged 8 commits into from
Jul 4, 2022

Conversation

PierreZ
Copy link
Member

@PierreZ PierreZ commented May 16, 2022

No description provided.

@coveralls
Copy link

coveralls commented May 16, 2022

Pull Request Test Coverage Report for Build 2370296968

  • 106 of 195 (54.36%) changed or added relevant lines in 3 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.6%) to 74.597%

Changes Missing Coverage Covered Lines Changed/Added Lines %
foundationdb/src/fdb_keys.rs 36 61 59.02%
foundationdb/src/mapped_key_values.rs 45 109 41.28%
Files with Coverage Reduction New Missed Lines %
foundationdb/src/tuple/subspace.rs 1 99.04%
foundationdb/src/directory/directory_layer.rs 8 92.49%
Totals Coverage Status
Change from base Build 2280667602: -0.6%
Covered Lines: 4255
Relevant Lines: 5704

💛 - Coveralls

@PierreZ PierreZ changed the title Support for Foundation DB 7.1 Support for FoundationDB 7.1 May 16, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #61 (b2ae4f1) into main (7a838a5) will decrease coverage by 0.72%.
The diff coverage is 55.38%.

❗ Current head b2ae4f1 differs from pull request most recent head 88b61ea. Consider uploading reports for the commit 88b61ea to get more accurate results

@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
- Coverage   83.52%   82.79%   -0.73%     
==========================================
  Files          24       24              
  Lines        4806     4936     +130     
==========================================
+ Hits         4014     4087      +73     
- Misses        792      849      +57     
Impacted Files Coverage Δ
foundationdb-gen/src/lib.rs 0.44% <ø> (ø)
foundationdb/src/lib.rs 100.00% <ø> (ø)
foundationdb/src/future.rs 65.90% <44.76%> (-8.99%) ⬇️
foundationdb/src/transaction.rs 90.02% <100.00%> (+0.78%) ⬆️
foundationdb/src/directory/directory_partition.rs 76.86% <0.00%> (-4.48%) ⬇️
foundationdb-bindingtester/src/main.rs 89.43% <0.00%> (-0.26%) ⬇️
foundationdb/src/directory/mod.rs 94.11% <0.00%> (ø)
foundationdb/src/directory/directory_layer.rs 94.32% <0.00%> (+2.23%) ⬆️

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 7a838a5...88b61ea. Read the comment docs.

}
}

pub fn end_range(&self) -> &[u8] {
Copy link
Collaborator

@Speedy37 Speedy37 May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this returns a KeySelector ?

}
}

pub fn begin_range(&self) -> &[u8] {
Copy link
Collaborator

@Speedy37 Speedy37 May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this returns a KeySelector ?

}
impl Eq for FdbMappedValue {}

pub struct FdbMappedValue {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit weird to declare it after its impls

@Speedy37
Copy link
Collaborator

Speedy37 commented May 18, 2022

I'm still not convinced features/macros are the best way to handle fdb versions.

I wonder if version features should be additives,

For example:
710, adds 700, 630, 620, 610, 600, 520, 510

In the code we would have to care about cfg! ordering and exclusion.

Example:

// From v600 to v630 included (i.e v600 to v700 not included)
cfg!(all(feature = "fdb-6_0", not(feature = "fdb-7_0")))
// v600 only
cfg!(all(feature = "fdb-6_0", not(feature = "fdb-6_1")))
// From v600 to now
cfg!(feature = "fdb-6_0")

@PierreZ
Copy link
Member Author

PierreZ commented May 19, 2022

Thanks for the review, your comments are in my TODO list, so I will fix them.

I'm still not convinced features/macros are the best way to handle fdb versions.

I wonder if version features should be additives,

For example: 710, adds 700, 630, 620, 610, 600, 520, 510

In the code we would have to care about cfg! ordering and exclusion.

Example:

// From v600 to v630 included (i.e v600 to v700 not included)
cfg!(all(feature = "fdb-6_0", not(feature = "fdb-7_0")))
// v600 only
cfg!(all(feature = "fdb-6_0", not(feature = "fdb-6_1")))
// From v600 to now
cfg!(feature = "fdb-6_0")

I agree. This is one of the reason the PR is in draft, I want to test a few things.

@PierreZ PierreZ added the correctness run hundred of iterations on the bindingTester during CI label May 23, 2022
@PierreZ PierreZ marked this pull request as ready for review May 23, 2022 09:52
@PierreZ
Copy link
Member Author

PierreZ commented May 23, 2022

PR ready to be reviewed,I will add tenant support in a separate PR

@PierreZ PierreZ requested a review from Speedy37 May 25, 2022 13:17
@PierreZ PierreZ merged commit 9c6716d into foundationdb-rs:main Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
correctness run hundred of iterations on the bindingTester during CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants