[indexer-alt] add prune impls for each pipeline #20635
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| async fn prune(range: PrunableRange, conn: &mut db::Connection<'_>) -> Result<usize> { | ||
| let (from, to) = range.containing_epochs(); | ||
| let filter = kv_epoch_starts::table | ||
| .filter(kv_epoch_starts::epoch.between(from as i64, to as i64 - 1)); |
There was a problem hiding this comment.
I have not fully convinced myself that pruning epoch grained tables will just work like this, even when our watermark and retention are checkpoint grained. I will add a test tomorrow to make sure.
There was a problem hiding this comment.
Indeed -- and I would say that if this implementation doesn't behave correctly, it would be good to change the helper function in PrunableRange so that all prune impls can follow the same pattern (regardless of whether they are epoch-, checkpoint-, or transaction-grained), rather than change the epoch-grained impls to have a slightly different structure.
There was a problem hiding this comment.
This looks great, thanks @emmazzz. There are some suggested changes on @wlmyng's PR that may affect this one (how the epoch helpers are implemented might introduce an off-by-one difference in this PR, and there is also a suggestion about changing the PrunableRange interface to move the responsibility to translate bounds into the individual prune impls).
I'll leave it with you and @wlmyng to coordinate those changes and then land both PRs!
There was a problem hiding this comment.
Given we can now delete by tx sequence number, should we get rid of the index on cp_sequence_number and write this prune impl based on the tx_interval?
There was a problem hiding this comment.
Do we actually have an index on cp_sequence_number? Seems like it is a field on the table, but no corresponding index. We might as well proceed with implementation based on tx_interval
There was a problem hiding this comment.
Ah, I must've missed something ... it looks like kv_transactions has only the cp_sequence_number field. So we'd need to backfill that. And since the primary key is on tx_digest, we'd need to introduce an index on tx_sequence_number
| async fn prune(range: PrunableRange, conn: &mut db::Connection<'_>) -> Result<usize> { | ||
| let (from, to) = range.containing_epochs(); | ||
| let filter = kv_epoch_starts::table | ||
| .filter(kv_epoch_starts::epoch.between(from as i64, to as i64 - 1)); |
There was a problem hiding this comment.
Indeed -- and I would say that if this implementation doesn't behave correctly, it would be good to change the helper function in PrunableRange so that all prune impls can follow the same pattern (regardless of whether they are epoch-, checkpoint-, or transaction-grained), rather than change the epoch-grained impls to have a slightly different structure.
wlmyng
force-pushed
the
indexer-alt-cp-mapping-for-pruning
branch
from
160d0a8 to
6cabf8a
Compare
wlmyng
force-pushed
the
indexer-alt-prune-impls
branch
from
2ce69cb to
438479b
Compare
wlmyng
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
wlmyng
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
wlmyng
force-pushed
the
indexer-alt-cp-mapping-for-pruning
branch
3 times, most recently
from
92c75e5 to
87504e7
Compare
wlmyng
force-pushed
the
indexer-alt-prune-impls
branch
from
b07a86e to
4726c70
Compare
wlmyng
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
wlmyng
force-pushed
the
indexer-alt-prune-impls
branch
from
4726c70 to
e8252bf
Compare
wlmyng
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| @@ -57,4 +60,16 @@ impl Handler for EvEmitMod { | |||
| .execute(conn) | |||
| .await?) | |||
| } | |||
|
|
|||
| async fn prune(from: u64, to: u64, conn: &mut db::Connection<'_>) -> Result<usize> { | |||
There was a problem hiding this comment.
Could you update the to to to_exclusive in all implementations?
| } = tx_interval(conn, from..to).await?; | ||
|
|
||
| let filter = ev_emit_mod::table | ||
| .filter(ev_emit_mod::tx_sequence_number.between(from_tx as i64, to_tx as i64 - 1)); |
There was a problem hiding this comment.
The to_tx is the first tx of the to_exclusive checkpoint and should not be pruned
wlmyng
force-pushed
the
indexer-alt-cp-mapping-for-pruning
branch
from
87504e7 to
4e0e03b
Compare
wlmyng
force-pushed
the
indexer-alt-prune-impls
branch
from
e8252bf to
040df88
Compare
wlmyng
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
wlmyng
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
wlmyng
force-pushed
the
indexer-alt-prune-impls
branch
from
9fd872e to
5ef7c04
Compare
wlmyng
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
Description
Added
pruneimplementations for pipeline inside indexer alt schema, built upon Will's cp mapping PR.Test plan
Will add tests.
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.