Implement consistency pruning in the builit-in pruner #20702
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| use crate::consistent_pruning::{PruningInfo, PruningLookupTable}; | ||
|
|
||
| #[derive(Default)] | ||
| pub(crate) struct ObjInfo { | ||
| pruning_lookup_table: Arc<PruningLookupTable>, | ||
| } |
There was a problem hiding this comment.
I think I might be missing something, but I don't see the consistent_pruning changes in this PR, or on main?
There was a problem hiding this comment.
Ah forgot to include the file
| async fn prune(&self, from: u64, to: u64, conn: &mut db::Connection<'_>) -> Result<usize> { | ||
| use sui_indexer_alt_schema::schema::obj_info::dsl; | ||
|
|
||
| let to_prune = self.pruning_lookup_table.take(from, to)?; |
There was a problem hiding this comment.
seems sensible enough for pruner to take self. The handler will know best the shape of the data it needs to do any consistent pruning, would likely be unwieldy otherwise
lxfind
force-pushed
the
indexer-alt-add-pruner-data-to-obj-info
branch
from
9196bde to
e2b70f4
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
indexer-alt-add-pruner-data-to-obj-info
branch
from
e2b70f4 to
cb2f77e
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
indexer-alt-add-pruner-data-to-obj-info
branch
from
cb2f77e to
0e652b0
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
There was a problem hiding this comment.
I like it overall! The only bit that I want to think about is how it interacts with resumption and the first checkpoint logic, but I think that is more to do with me being unsatisfied with how that bit of code works already -- I think this approach is pretty nice and clean.
| @@ -27,6 +27,10 @@ pub trait Processor { | |||
| /// How much concurrency to use when processing checkpoint data. | |||
| const FANOUT: usize = 10; | |||
|
|
|||
| /// Whether the pruner requires processed values in order to prune. | |||
| /// This will determine the first checkpoint to process when we start the pipeline. | |||
| const PRUNING_REQUIRES_PROCESSED_VALUES: bool = false; | |||
There was a problem hiding this comment.
This is kind of an unfortunate consequence of this approach, but I'm coming around to it, with some small tweaks:
- This feature is unique to concurrent pipelines (as is pruning), so we should put it on the
concurrent::Handlertrait (although I see now above why you've put it onProcessorbased on where in the codebase you fetch the watermark). - I would name it for exactly what it controls (resuming from the pruner watermark), rather than what we might use it for (back-channeling values from the processor to the pruner). How about
RESUME_FROM_PRUNER_HIor something?
There was a problem hiding this comment.
To me "resuming from the pruner watermark" is an implementation detail that would actually be difficult for pipeline builders to understand. What they care about is the fact that they want to use processed data to do pruning, and the need to resume ingestion from pruner hi is a side-effect.
lxfind
force-pushed
the
indexer-alt-add-pruner-data-to-obj-info
branch
from
0e652b0 to
52ea442
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
changed the title
Description
This PR implements consistent pruning within the built-in pruner for concurrent pipelines.
Instead of relying on a separate pipeline to do pruning, this PR adds shared data between the processor and pruner for obj_info.
Note that this PR isn't finished yet, sending our early to make sure this is not in the wrong direction.
To implement this, I also need to make pruner take
selfso that it has access to shared data. The first checkpoint of indexer also needs special care to make sure for any pipeline that requires processed data to prune, we must start processing at the pruner watermark instead of commit watermark.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.