Skip to content

Implemented AsyncInMemoryDatabase::set in terms of ::batch_set#425

Closed
cryo28 wants to merge 1 commit intofacebook:mainfrom
cryo28:main
Closed

Implemented AsyncInMemoryDatabase::set in terms of ::batch_set#425
cryo28 wants to merge 1 commit intofacebook:mainfrom
cryo28:main

Conversation

@cryo28
Copy link
Contributor

@cryo28 cryo28 commented Mar 7, 2024

The implementation of AsyncInMemoryDatabase::set and batch_set contain a lot of duplicate code.

Tested with:

cargo test test_in_memory_db

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 7, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.43%. Comparing base (24b11de) to head (d759c17).

Files Patch % Lines
akd/src/storage/memory.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   87.99%   88.43%   +0.43%     
==========================================
  Files          39       39              
  Lines        9054     9198     +144     
==========================================
+ Hits         7967     8134     +167     
+ Misses       1087     1064      -23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@kevinlewi kevinlewi left a comment

Choose a reason for hiding this comment

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

Awesome, thanks and LGTM. However, seems like there is a clippy error that is on a line which is unrelated to your changes, but if you can fix it so that CI tests pass, that would be great!

https://github.com/facebook/akd/actions/runs/8195091568/job/22412621676?pr=425#step:4:556

@kevinlewi
Copy link
Contributor

Sorry, I landed #426 first which also fixed the clippy error -- can you rebase on main and force-push again? Should fix the CI issue

@cryo28
Copy link
Contributor Author

cryo28 commented Mar 16, 2024

It seems like force updates don't re-trigger workflows. Not sure what I should do? Should I recreate the PR or add some dummy commit to the existing one?

@cryo28 cryo28 closed this Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants