feat: Added in-memory storage for testing purposes #59
Conversation
|
👋 I see @tnull was un-assigned. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
Generally goes into the right direction, but we def. need to avoid re-allocating everything on every operation.
Harshdev098
force-pushed
the
memory_store
branch
2 times, most recently
from
4980a75 to
25d57e3
Compare
|
@tnull Have done the required changes |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Looks much better, but I think we still need to handle global_version properly, even if we're currently not using it client-side.
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
Harshdev098
force-pushed
the
memory_store
branch
from
25d57e3 to
9012e95
Compare
Harshdev098
force-pushed
the
memory_store
branch
from
9012e95 to
3b434d0
Compare
|
@tankyleo Can you please review it! |
|
🔔 3rd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tankyleo! This PR has been waiting for your review. |
rust/impls/src/in_memory_store.rs
Outdated
| version: record.version, | ||
| }), | ||
| }) | ||
| } else if request.key == GLOBAL_VERSION_KEY { |
There was a problem hiding this comment.
Looks like by the time we are here, we know the GLOBAL_VERSION_KEY does not have a value, otherwise guard.get would have returned Some previously. We can just return the GetObjectResponse below directly with version: 0.
There was a problem hiding this comment.
@Harshdev098 double checking things here, we still have this second branch the same as before ?
There was a problem hiding this comment.
Shouldn't we still return early if request.key == GLOBAL_VERSION_KEY rather than always first making the lookup?
There was a problem hiding this comment.
The first lookup handles cases where the GLOBAL_VERSION_KEY is some non-zero value. We want to check whether it's been set to some value in the map before returning the initial value in this branch.
Harshdev098
force-pushed
the
memory_store
branch
from
3b434d0 to
e0c31bb
Compare
|
@tankyleo Have done with the required changes! Can you please review it |
Harshdev098
force-pushed
the
memory_store
branch
from
8003119 to
5898609
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
When testing integration with LDK Node locally I found that the tests are currently failing. I now opened #62 to add LDK Node integration tests to our CI here. It would be great if that could land first, and we could also add a CI job for the in-memory store as part of this PR then, ensuring the implementation actually works as expected.
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
@Harshdev098 Please rebase now that #62 landed to make use of the new CI checks here. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
Can you also rephrase your commit messages to use the imperative form ? See https://cbea.ms/git-commit/
rust/impls/src/in_memory_store.rs
Outdated
| Err(VssError::NoSuchKeyError("Requested key not found.".to_string())) | ||
| }; | ||
|
|
||
| result |
There was a problem hiding this comment.
nit: no need for the extra result variable here, just return from the if statement directly
rust/impls/src/in_memory_store.rs
Outdated
| let storage_prefix = format!("{}#{}#", user_token, store_id); | ||
| let prefix_len = storage_prefix.len(); | ||
|
|
||
| let mut all_items: Vec<KeyValue> = guard |
There was a problem hiding this comment.
You did not address the earlier feedback, we are still making an extra allocation here when we only need one
rust/impls/src/in_memory_store.rs
Outdated
| }) | ||
| .collect(); | ||
|
|
||
| all_items.sort_by(|a, b| a.key.cmp(&b.key)); |
There was a problem hiding this comment.
Again as I mentioned further above, we now don't need this sort here let's delete it
rust/impls/src/in_memory_store.rs
Outdated
|
|
||
| let mut all_items: Vec<KeyValue> = guard | ||
| .iter() | ||
| .filter(|(storage_key, _)| storage_key.starts_with(&storage_prefix)) |
There was a problem hiding this comment.
here let's filter out the keys that do not match the prefix as well as the global version key
rust/impls/src/in_memory_store.rs
Outdated
| global_version = Some(self.get_current_global_version(&guard, &user_token, &store_id)); | ||
| } | ||
|
|
||
| let storage_prefix = format!("{}#{}#", user_token, store_id); |
There was a problem hiding this comment.
let's set this to build_storage_key(&user_token, &store_id, &key_prefix); so that we can do a single filter call to filter out all the keys.
rust/impls/src/in_memory_store.rs
Outdated
| let mut all_items: Vec<KeyValue> = guard | ||
| .iter() | ||
| .filter(|(storage_key, _)| storage_key.starts_with(&storage_prefix)) | ||
| .filter_map(|(storage_key, record)| { |
There was a problem hiding this comment.
Let's not do any filtering in this function, just map
rust/impls/src/in_memory_store.rs
Outdated
|
|
||
| let mut all_items: Vec<KeyValue> = guard | ||
| .iter() | ||
| .filter(|(storage_key, _)| storage_key.starts_with(&storage_prefix)) |
There was a problem hiding this comment.
After this call, we can do .skip(offset).take(limit).map...
rust/impls/src/in_memory_store.rs
Outdated
| all_items.sort_by(|a, b| a.key.cmp(&b.key)); | ||
|
|
||
| let page_items: Vec<KeyValue> = | ||
| all_items.iter().skip(offset).take(limit).cloned().collect(); |
There was a problem hiding this comment.
As mentioned earlier, this skip(offset).take(limit) can be folded into the first iterator chain.
rust/server/vss-server-config.toml
Outdated
| [server_config] | ||
| host = "127.0.0.1" | ||
| port = 8080 | ||
| store_type = "postgres" # "postgres" for using postgresql and "in_memory" for testing purposes |
There was a problem hiding this comment.
in_memory is not valid any longer
rust/impls/src/models.rs
Outdated
| use chrono::Utc; | ||
|
|
||
| /// A record stored in the VSS database. | ||
| pub struct VssDbRecord { |
There was a problem hiding this comment.
Let's delete all the pub here this is not part of the public API, and let's move this to the tail end of src/lib.rs, this is a better spot for this stuff - we can delete model.rs
| let limit = std::cmp::min(page_size, LIST_KEY_VERSIONS_MAX_PAGE_SIZE) as usize; | ||
|
|
||
| let offset: usize = | ||
| request.page_token.as_ref().and_then(|s| s.parse::<usize>().ok()).unwrap_or(0); |
There was a problem hiding this comment.
If we cannot parse the token, that is an error - currently we accept it and set offset to 0.
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
Harshdev098
force-pushed
the
memory_store
branch
2 times, most recently
from
2d78e44 to
6ee3bc8
Compare
Harshdev098
force-pushed
the
memory_store
branch
from
6ee3bc8 to
e657210
Compare
rust/impls/src/in_memory_store.rs
Outdated
| version: record.version, | ||
| }), | ||
| }) | ||
| } else if request.key == GLOBAL_VERSION_KEY { |
There was a problem hiding this comment.
Shouldn't we still return early if request.key == GLOBAL_VERSION_KEY rather than always first making the lookup?
|
🔔 4th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tankyleo! This PR has been waiting for your review. |
Harshdev098
force-pushed
the
memory_store
branch
from
e657210 to
257c45b
Compare
5 participants
Have added in_memory store for testing purpose.
We can edit config file to use specific store either postgresql or memory