-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Storage][Sharding] Sharded state merkle pruner. #7857
Conversation
8a50c73
to
3258416
Compare
@@ -27,16 +28,25 @@ mod test; | |||
|
|||
pub const STATE_MERKLE_PRUNER_NAME: &str = "state_merkle_pruner"; | |||
|
|||
static POOL: Lazy<rayon::ThreadPool> = Lazy::new(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider a more specific name like TREE_PRUNER_THREAD_POOL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// to the DB. | ||
fn prune_state_merkle_shard( | ||
&self, | ||
db: &DB, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you name this DB? too many DBs, sometime, hard to know which DB this refers to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
let shard_min_readable_version = self.get_shard_progress(shard_id); | ||
if shard_min_readable_version != target_version { | ||
assert_lt!(shard_min_readable_version, target_version); | ||
self.update_shard_progress(shard_id, target_version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, do we want to update shard progress once the prune is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The db progress is updated after, the in memory one is updated before, to tell the upper layer the data is not readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I said above has changed now (after #8532). Now we manage the "min_readable_version" in the pruner manager (updated before the work, and this is the value we serve to outside), and we manager the "progress" in the pruner structs (updated after the work is done).
) -> Result<Option<Version>> { | ||
let batch = SchemaBatch::new(); | ||
let next_version = self.prune_state_merkle_shard( | ||
self.state_merkle_db.metadata_db(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the top level SMT stored in metadata_db?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
f1d8b64
to
78c349d
Compare
def5a60
to
a438cea
Compare
bf133f8
to
b1bef72
Compare
@@ -102,17 +104,19 @@ where | |||
|
|||
// used only by blanket `initialize()`, use the underlying implementation instead elsewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
} | ||
|
||
pub(in crate::pruner) fn progress(&self) -> Result<Version> { | ||
Ok(get_progress(&self.metadata_db, &S::tag(None))?.unwrap_or(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From previous PR, but in get_progress()
we should call DbMetadataValue::expect_version()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
fn name() -> &'static str; | ||
} | ||
|
||
impl StaleNodeIndexSchemaTrait for StaleNodeIndexSchema { | ||
fn tag() -> DbMetadataKey { | ||
DbMetadataKey::StateMerklePrunerProgress | ||
fn tag(shard_id: Option<u8>) -> DbMetadataKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my fault, but rename, maybe progress_metadata_key()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pub(in crate::pruner) fn new(metadata_db: Arc<DB>) -> Self { | ||
Self { | ||
metadata_db, | ||
next_version: AtomicVersion::new(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man I have to say this field is useless.. looking at the code it's always been overwritten by current_progress
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, for most of the time it's always be larger than current_progress, and it's overwritten by either the second smallest version that is larger than current_progress (the smallest version >= current_progress get pruned), or the target_version (if we've already reach the end of stale index in this round)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay.. needed a vc session to understand what you are saying, but seems legit 😞
b1bef72
to
e645ab5
Compare
pub(in crate::pruner) fn new(metadata_db: Arc<DB>) -> Self { | ||
Self { | ||
metadata_db, | ||
next_version: AtomicVersion::new(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay.. needed a vc session to understand what you are saying, but seems legit 😞
storage/aptosdb/src/utils/mod.rs
Outdated
None | ||
}, | ||
) | ||
Ok(if let Some(v) = db.get::<DbMetadataSchema>(progress_key)? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use .map()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
e645ab5
to
2d6740f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2d6740f
to
d7ff448
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
Test Plan
Tested in executor-benchmark.