-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[indexer][watermarks][1/n] Modify PruningOptions to point to a toml file of epochs_to_keep and optional per-table overrides #19637
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
c58b452
to
ad1fc27
Compare
Whoa.... how'd that happen |
8d6aa35
to
199532b
Compare
855e5e0
to
bf34b15
Compare
b8f4fea
to
5cdcd79
Compare
5cdcd79
to
5065a0a
Compare
98aeae3
to
4c862a4
Compare
5065a0a
to
1157e65
Compare
4c862a4
to
5399f84
Compare
1157e65
to
84392bf
Compare
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.
Left some comments but mostly good. Same comment re objects_history
override as Ge and Xun.
crates/sui-indexer/src/config.rs
Outdated
|
||
/// Updates the `policies` field with the default retention policy for all tables that do not | ||
/// have an override specified. | ||
pub fn finalize(mut self) -> Self { |
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.
I find it a bit odd to have both a new
and a finalize
method when no changes are made between their calls. I understand the purpose might be to reuse the override config map as a complete set, combining both default and override pruning configurations. But what do you think of simplifying this by keeping the struct as just the config, and having a function that takes the config and returns a hashmap with the complete retention policies like this?
pub struct RetentionConfig {
/// Default retention policy for all tables.
pub epochs_to_keep: u64,
/// A user only needs to explicitly list tables that should override the
/// default retention.
#[serde(default)]
pub overrides: HashMap<PrunableTable, u64>,
}
pub fn retention_policies_from_config(config: RetentionConfig
) -> HashMap<PrunableTable, u64> {
let RetentionConfig {
epochs_to_keep, overrides
}
let res = overrides;
for table in PrunableTable::iter() {
res.entry(table).or_insert(self.epochs_to_keep);
}
res
}
let parent_tables_from_db: HashSet<_> = result.into_iter().map(|t| t.table_name).collect(); | ||
|
||
for key in PrunableTable::iter() { | ||
if !parent_tables_from_db.contains(key.as_ref()) { |
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.
In our chat earlier, I mentioned that this check might not be necessary, but I now think it’s actually useful. Consider a scenario where we add a pruning config for a new table prematurely. Without this check, we might forget to review the config later when the table is officially added, leading to potential issues. Having the check ensures that we verify the pruning configuration at the right time, preventing errors down the line.
## Description Currently, tests that require spinning up an indexer do so through `start_test_indexer_impl` or `start_test_indexer`. This is further complicated by a single `start` function that accepts a `ReaderWriterConfig`. We can simplify this by exposing two functions with optional parameters that can be configured by the caller. Part of a stack of PRs for watermarks 1. simplify setting up test indexer: #19663 2. update pruner config: #19637 3. committer writes upper bounds #19649 4. pruner writes lower bounds: #19650 5. pruner prunes (wip) ## Test plan How did you test the new or updated feature? --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
make life easier, requires epochs_to_keep with optional overrides. otherwise, no retention policy at all strumming the pruning config. pruner.rs file will define prunable tables. the toml file when parsed into Rust config will warn if it tries to name any variants not supported by indexer code. additionally, there's also a check to make sure that prunable tables actually exist in the db
84392bf
to
1bbe93b
Compare
Description
Modify
PruningOptions
to point to a toml file that must specifyepochs_to_keep
and an optional[overrides]
portion of per-table overrides.pruner.rs file will define prunable tables. the toml file when parsed into Rust config will warn if it tries to name any variants not supported by indexer code. additionally, there's also a check to make sure that prunable tables actually exist in the db
Part of a stack of PRs for watermarks
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.