Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chain: gc partial chunks on archival nodes #6439

Merged
merged 17 commits into from
Apr 6, 2022
Merged

chain: gc partial chunks on archival nodes #6439

merged 17 commits into from
Apr 6, 2022

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Mar 15, 2022 •
edited
Loading

Start garbage collecting ColPartialChunks and ColInvalidChunks on
archival nodes. The former is quite sizeable column and its data can
be recovered from ColChunks. The latter is only needed when operating
at head.

Note that this is likely insufficient for the garbage collection to
happen in reasonable time (since with current default options we’re
garbage collecting only two heights at a time). It’s best to clean
out the two columns.

Issue: #6242

matklad
matklad reviewed Mar 16, 2022
View reviewed changes
chain/chain/src/store.rs Outdated Show resolved Hide resolved
@matklad matklad mentioned this pull request Mar 16, 2022
Open
matklad
matklad reviewed Mar 16, 2022
View reviewed changes
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absence of tests is concerning. I guess it's the usual "we don't have infra to test this conveniently", but the logic here looks quite finicky...

@mina86
Copy link
Contributor Author

mina86 commented Mar 16, 2022 •
edited
Loading

Absence of tests is concerning. I guess it's the usual "we don't have infra to test this conveniently", but the logic here looks quite finicky...

Yeah, I’ve tested this manually and pytest/tests/sanity/block_sync_archival.py checks that the node is able to fill in those requests but there is currently no test that checks the gc is happening. I am contemplating a few approaches so I am planning to add something.

@mina86 mina86 force-pushed the gc branch 2 times, most recently from 4cc84bb to 60e4960 Compare March 17, 2022 15:36
mzhangmzz
mzhangmzz reviewed Mar 18, 2022
View reviewed changes
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
@mina86
Copy link
Contributor Author

mina86 commented Mar 22, 2022

@mzhangmzz, PTAL

mzhangmzz
mzhangmzz approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@mzhangmzz mzhangmzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

bowenwang1996
bowenwang1996 reviewed Mar 23, 2022
View reviewed changes
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not very comfortable that we don't have a unit test for this. It feels to me that should be relatively easy to do with TestEnv

@mina86 mina86 mentioned this pull request Mar 24, 2022
Merged
mina86 added 2 commits March 27, 2022 17:31
@mina86
remove archive_gc_partial_chunks, make it always on
d98b8f1 
So I’ve been thinking about rolling this feature out and started
wondering about how third party operators will deal with it.  With the
need to run recompress and then setting this option I concluded that
in the end adding the option just leaves window open for someone to
end up with a messy archival node.  On top of that, my current
thinking is to cut 1.25.1 which would act as an opt-in step.
@mina86
Merge remote-tracking branch 'up/master' into HEAD
a9b30bc
@mina86
Copy link
Contributor Author

mina86 commented Mar 27, 2022

It’s kinda tested a bit in the Python test, at the moment so at least there’s that.

@mina86 mina86 changed the title chain: add option to gc partial chunks on archival nodes chain: gc partial chunks on archival nodes Mar 27, 2022
mm-near
mm-near previously requested changes Mar 29, 2022
View reviewed changes
pytest/tests/sanity/block_sync_archival.py Outdated Show resolved Hide resolved
pytest/tests/sanity/block_sync_archival.py Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
@mina86 mina86 requested a review from mm-near March 29, 2022 12:18
@mina86 mina86 dismissed mm-near’s stale review March 29, 2022 12:18

Addressed comments.

mm-near
mm-near approved these changes Mar 31, 2022
View reviewed changes
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
pytest/lib/cluster.py Show resolved Hide resolved
pytest/tests/sanity/block_sync_archival.py Show resolved Hide resolved
pytest/tests/sanity/block_sync_archival.py Outdated Show resolved Hide resolved
@mina86 mina86 mentioned this pull request Apr 4, 2022
Closed
@mina86 mina86 requested a review from a team as a code owner April 6, 2022 17:29
@near-bulldozer near-bulldozer bot merged commit 6be2e0e into near:master Apr 6, 2022
@mina86 mina86 deleted the gc branch April 6, 2022 17:48
mina86 added a commit to mina86/nearcore that referenced this pull request Apr 7, 2022
@mina86
chain: gc partial chunks on archival nodes (near#6439)
8ba84f3 
This is commit 6be2e0e upstream.

Start garbage collecting ColPartialChunks and ColInvalidChunks on
archival nodes.  The former is quite sizeable column and its data can
be recovered from ColChunks.  The latter is only needed when operating
at head.

Note that this is likely insufficient for the garbage collection to
happen in reasonable time (since with current default options we’re
garbage collecting only two heights at a time).  It’s best to clean
out the two columns.

Issue: near#6242
mina86 added a commit to mina86/nearcore that referenced this pull request Apr 8, 2022
@mina86
chain: allow chain_tail to be set w/o tail being set on archival nodes
86e0e2d 
Since commit 6be2e0e: ‘gc partial chunks on archival nodes (near#6439)’,
archival nodes set chunk_tail without setting tail.  However, store
validation expects both of those to be set or unset.  Change the code
to allow unset tail on archival nodes.

Issue: near#6242
@mina86 mina86 mentioned this pull request Apr 8, 2022
Merged
near-bulldozer bot pushed a commit that referenced this pull request Apr 11, 2022
@mina86
chain: allow chain_tail to be set w/o tail being set on archival nodes (
6a00c11 
#6563)

Since commit 6be2e0e: ‘gc partial chunks on archival nodes (#6439)’,
archival nodes set chunk_tail without setting tail.  However, store
validation expects both of those to be set or unset.  Change the code
to allow unset tail on archival nodes.

Issue: #6242
pompon0 pushed a commit that referenced this pull request Apr 15, 2022
@mina86 @pompon0
chain: allow chain_tail to be set w/o tail being set on archival nodes (
d4a174b 
#6563)

Since commit 6be2e0e: ‘gc partial chunks on archival nodes (#6439)’,
archival nodes set chunk_tail without setting tail.  However, store
validation expects both of those to be set or unset.  Change the code
to allow unset tail on archival nodes.

Issue: #6242
@gmilescu gmilescu mentioned this pull request Nov 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenwang1996 bowenwang1996 bowenwang1996 left review comments

@matklad matklad matklad left review comments

@mm-near mm-near mm-near approved these changes

@mzhangmzz mzhangmzz mzhangmzz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mina86 @matklad @bowenwang1996 @mzhangmzz @mm-near