chain: ShardManager: add ability to serve partial chunks from ShardChunk #6377
Conversation
Add ability to respond to PartialEncodedChunkRequest from ShardChunk objects in addition to PartialEncodedChunk. In practice this is currently dead code since there is no scenario in which the former is in the storage while the latter isn’t but the plan is to start garbage collecting ColPartialChunks column at which point we’ll have to serve requests from data in ColChunks column. Issue: #6242
mina86
requested review from
bowenwang1996,
matklad,
mm-near and
mzhangmzz
as code owners
chain/chunks/src/lib.rs
Outdated
| @@ -1075,6 +1089,136 @@ impl ShardsManager { | |||
| ) | |||
| } | |||
|
|
|||
| fn prepare_partial_encoded_chunk_response_from_chunk( | |||
There was a problem hiding this comment.
maybe we should move it to a separate file (to keep this lib.rs a little bit smaller?)
There was a problem hiding this comment.
It’s not immediately obvious to be me what should be moved to separate file though to be honest. Code that handles partial encoded chunk requests is less than 250 lines so moving just it wouldn’t really help reduce the size of lib.rs which is 2700 lines. This would need some more care and thought to figure out which of the methods in the struct belong together in a separate file.
There was a problem hiding this comment.
Yeah, I don't think this makes sense to move to a separate file, as it seems to be pretty entangled with the rest.
The easiest wins in terms of keeping files small and logic clean are usually around various small helpers. Things like RequestPool or SealsManager seem like better candidates for extraction.
| ) { | ||
| debug!(target: "chunks", "Received partial encoded chunk request for {:?}, part_ordinals: {:?}, shards: {:?}, I'm {:?}", request.chunk_hash.0, request.part_ords, request.tracking_shards, self.me); | ||
|
|
||
| let response = if let Some(entry) = self.encoded_chunks.get(&request.chunk_hash) { | ||
| Self::prepare_partial_encoded_chunk_response_from_cache(request, entry) | ||
| } else if let Ok(partial_chunk) = chain_store.get_partial_chunk(&request.chunk_hash) { | ||
| Self::prepare_partial_encoded_chunk_response_from_partial(request, partial_chunk) | ||
| } else if let Ok(chunk) = chain_store.get_chunk(&request.chunk_hash).map(|ch| ch.clone()) { | ||
| // Note: we need to clone the chunk because otherwise we would be |
There was a problem hiding this comment.
we should have metrics for all 3 cases here (can be done in separate PR)
There was a problem hiding this comment.
I agree with Marcin that we want to have some measurement of how long it takes to process a partial encode chunk part request by reconstructing the chunk parts, since it might not be cheap. Another question we need to answer after we have the measurement is whether we want to cache the results for some time, since the node requesting for these parts may resend the request again.
| @@ -33,3 +34,4 @@ assert_matches = "1.5.0" | |||
| [features] | |||
| byzantine_asserts = ["near-chain/byzantine_asserts"] | |||
| expensive_tests = [] | |||
| test_features = [] | |||
There was a problem hiding this comment.
comment. Also maybe we should call it 'reconstruct_partial_chunk_on_the_fly' or something like that?
There was a problem hiding this comment.
I’m planning to make GCing a configurable feature.
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
…unk (near#6377) This is commit 09041ec upstream. Add ability to respond to PartialEncodedChunkRequest from ShardChunk objects in addition to PartialEncodedChunk. In practice this is currently dead code since there is no scenario in which the former is in the storage while the latter isn’t but the plan is to start garbage collecting ColPartialChunks column at which point we’ll have to serve requests from data in ColChunks column. Issue: near#6242
Add ability to respond to PartialEncodedChunkRequest from ShardChunk
objects in addition to PartialEncodedChunk. In practice this is currently
dead code since there is no scenario in which the former is in the storage
while the latter isn’t but the plan is to start garbage collecting
ColPartialChunks column at which point we’ll have to serve requests from
data in ColChunks column.
Issue: #6242