[LogServer] Initial implementation for store path #1938
Conversation
There was a problem hiding this comment.
Thanks @AhmedSoliman for the PR. This looks good to me but I left couple of questions
|
|
||
| use crate::rocksdb_logstore::keys::{KeyPrefixKind, MetadataKey}; | ||
|
|
||
| pub(super) fn metadata_full_merge( |
There was a problem hiding this comment.
It took me some time to understand what this function do. And I am still not very sure if I got this correctly. Would be nice if you add some doc strings
There was a problem hiding this comment.
In case it helps, here's a test case from rust-rocksdb that demonstrates the feature (reasonably) concisely: https://github.com/rust-rocksdb/rust-rocksdb/blob/2bbcd68ca229b6ad98e77d87dac9cfe77fc9d961/tests/test_merge_operator.rs#L42-L77
| @@ -85,4 +90,259 @@ impl LogStore for RocksDbLogStore { | |||
| .map_err(RocksDbLogStoreError::from)?; | |||
| Ok(()) | |||
| } | |||
|
|
|||
| async fn load_loglet_state( | |||
There was a problem hiding this comment.
I don't see this one being used except in tests
There was a problem hiding this comment.
It's used in LogletStateMap::get_or_load(..) in log-server/src/metadata.rs:76
Note 1: This PR contains many todos an various incomplete bits that will be worked out in later PRs. This is highly experimental code and will be refined in the next iterations but I wanted to avoid accumulating code locally. Note 2: This enables log-server by default, so it's imperative that this isn't included in v1.1 release. When looking at this, consider the overall design strategy and don't get hang up on the nits or smaller details as they're likely to change anyway. What this achieves: - Starts log-server by default (role is now enabled by default) - Introduces Store/Release network messages for the log-server - Implements pipelined Store messages from sequencers, no backfill ability. - LogState management - Provisional trim infrastructure
AhmedSoliman
force-pushed
the
pr1938
branch
from
f6d1d7f
to
d0646f3
Compare
| // log-store marker | ||
| pub(super) const MARKER_KEY: &[u8] = b"storage-marker"; | ||
|
|
||
| // makes sure that it doesn't go unnoticed if this changed by mistake. |
|
|
||
| use crate::rocksdb_logstore::keys::{KeyPrefixKind, MetadataKey}; | ||
|
|
||
| pub(super) fn metadata_full_merge( |
There was a problem hiding this comment.
In case it helps, here's a test case from rust-rocksdb that demonstrates the feature (reasonably) concisely: https://github.com/rust-rocksdb/rust-rocksdb/blob/2bbcd68ca229b6ad98e77d87dac9cfe77fc9d961/tests/test_merge_operator.rs#L42-L77
| trace!(key = ?key, "metadata_full_merge"); | ||
| if key.kind() != KeyPrefixKind::TrimPoint { | ||
| error!(key = ?key, "Merge is only supported for trim-points"); | ||
| return None; |
There was a problem hiding this comment.
I was curious how RocksDB and the Rust layer handles this - merge_cf will return an error like Error { message: "Corruption: Merge operator failed" }. Basically, it's on us to never call merge with an unsupported operand.
| .map(LogletOffset::decode) | ||
| .unwrap_or(LogletOffset::INVALID); | ||
|
|
||
| for op in operands { |
There was a problem hiding this comment.
Nice! This is the first time I'm seeing this in the wild :-)
| return (Status::OutOfBounds, None); | ||
| } | ||
|
|
||
| let set_sequncer_in_metadata = if known_sequencer.is_none() { |
There was a problem hiding this comment.
typo: set_seqeuncer_in_metadata
| .inner() | ||
| .as_raw_db() | ||
| .raw_iterator_cf_opt(&data_cf, readopts); | ||
| // see to the max key that exists |
[LogServer] Initial implementation for store path
Note 1: This PR contains many todos an various incomplete bits that will be worked out in later PRs. This is highly experimental code and will be refined in the next iterations but I wanted to avoid accumulating code locally.
Note 2: This enables log-server by default, so it's imperative that this isn't included in v1.1 release.
When looking at this, consider the overall design strategy and don't get hang up on the nits or smaller details as they're likely to change anyway.
What this achieves:
Stack created with Sapling. Best reviewed with ReviewStack.