-
Notifications
You must be signed in to change notification settings - Fork 418
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
Add Layer::overlaps method and use it in count_deltas to avoid unnecessary image layer generation #3348
Closed
Closed
Add Layer::overlaps method and use it in count_deltas to avoid unnecessary image layer generation #3348
Changes from all commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
121b5e9
Add Layer::overlaps method and use it in count_deltas to avoid unness…
knizhnik 7d1e6ac
Remove wanrings
knizhnik c14cc5b
Store information about largest holes in layer map
knizhnik 9b2321e
Merge with main
knizhnik 8f23a5e
Store information about holes in delta layers in S3 index file
knizhnik 652fd5a
Remove excessive clone() when merging holes
knizhnik aa89e26
Fix unit tests building
knizhnik 119afc3
Make clippy happy
knizhnik f63c663
Add traces to debug physical layer size calculation
knizhnik 8b85811
Temporary disarm overlaps
knizhnik 4099101
Revert "Temporary disarm overlaps"
knizhnik 38754b2
Revert "Make clippy happy"
knizhnik 4f38224
Ad delay to test_on_demand_download
knizhnik c2be42b
Revert "Add traces to debug physical layer size calculation"
knizhnik 3100984
Make clippy happy
knizhnik 08f450e
Ignore errors in find_lsn_for_timestamp
knizhnik d7c09ca
Revert "Ignore errors in find_lsn_for_timestamp"
knizhnik c05095f
Temp: do not use information about holes in GC
knizhnik 0b32d14
Fix LayerMap::search method
knizhnik 0824044
Trnsacte index_part.json file in test_tenant_upgrades_index_json_from_v0
knizhnik 3dc4ab3
Add holes information while upgrading layer metadata
knizhnik eef2ab6
Make clippy happy
knizhnik 6c54d0d
Provide custom serializer for Hole
knizhnik d63a745
Propagate RequestContext
knizhnik 7c3c271
Store historic layers in separate set
knizhnik a58b703
Remove all occupied segments in layer map
knizhnik 161e0d8
Remove all occupied segments in layer map
knizhnik 5e2cea2
Rebase with main
knizhnik e020247
Cleanup after merge with main
knizhnik 88fc5c3
Minor refactoring
knizhnik 6aab1a9
Minor refactoring
knizhnik d89e5a7
Ignore load layer error in get_occupied_ranges
knizhnik be24282
Ignore load layer error in get_occupied_ranges
knizhnik 689ef9e
Sort layers in LayerMapInfo
knizhnik e068e48
Sort layers in LayerMapInfo
knizhnik 8051f94
Update pageserver/src/tenant/layer_map.rs
knizhnik 04c81fc
Add test for format version 2 of index_part.json
knizhnik 4b3fe74
Try to use wait_for_upload in test_ondemand_download_timetravel
knizhnik ed44d66
Restore v1_indexpart_is_parsed test
knizhnik ad4d678
Merge with main
knizhnik 284d167
Restore sleep in test_ondemand_download.py
knizhnik e9b0e43
Update pageserver/src/tenant/layer_map.rs
knizhnik 6e5efc5
Treate Arc as raw pointers in hash implementation for LayerRef
knizhnik d78fbb0
Mak eclippy happy
knizhnik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -264,6 +264,8 @@ pub enum TaskKind { | |
|
||
DebugTool, | ||
|
||
Benchmark, | ||
|
||
#[cfg(test)] | ||
UnitTest, | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We can plumb
ctx
everywhere if it's needed. But I wonder what particular change in this PR required the need to plumbctx
. Why is it unavoidable?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.
if we really need to add
ctx
to all these places, please extract that to a separate commit, for easier reviewThere 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.
Or a separate PR, but I think this depends on the "should LayerMap receive layers with full hole information or can it calculate them on demand". I think this would be possible to side-step by requiring that the layers have the metadata before being put into the LayerMap.
See: https://github.com/neondatabase/neon/pull/3348/files#r1097689494
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.
First reason: The layer map is just a data structure. It should be possible to separate it into its own lib crate. Why? When I rewrote the layer map I spent 1 week writing layer map code and 2 months dealing with the lack of separation between the layer map and the caller (no tests, no documented assumptions, too many requirements of a minimum viable implementation, etc.) What we're doing here with
ctx
is equivalent to plumbingctx
into theHashMap
implementation because we want to read a delta file insideDeltaLayer::hash
. It's bloated. It's not modular.Second reason: The need to plumb
ctx
for this PR reveals that the layer map needed some kind of access to some kind of expensive operation worth tracking. When someone reads this code, their guess would be that it either:a) it reads the holes from the header of the delta layer
b) it reads the entire delta layer to construct the holes
c) it downloads layers (pls no!)
A paranoid reader will assume it's (c) and will have to read a lot more code just to make sure. A careless reader will assume it's (a) and move on. In reality it's (b), so nobody is right. Reading code should not be an adventure. The code should make it obvious what's going on and what can be improved. The reader's attention should be on the important bits. Given how complicated the layer map already is, I'd try to remove ctx completely so that there would be no doubt, but if we have to hack something I'd at least be very explicit with comments and TODOs explaining how it can be done better.
Overall, I agree that if we have
ctx
we should plumb it in all methods proactively. But there are exceptions:get_timezone
function will raise red flags. We're now passing error handling logic into a function that should never attempt to do any of those things that need the error handling.It seems like an avoidable problem given that we actually compute the holes explicitly whenever we create a delta layer and add it to the layer map. Why not keep or pass that information?
https://github.com/neondatabase/neon/blob/56af67b5931569525b80256b4a9549d05e2fbc9a/pageserver/src/tenant/timeline.rs#L2422-L2429
I'd be fine with at least documenting
// HACK this can be done a lot better
and moving on, if the solution is actually a lot more involved than I think.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.
😆
FWIW, I agree 100% on your reasoning.
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.
Line 2242 that I linked to, says
let holes = new_delta.get_holes(ctx)?;
. Is this not calculating holes?Let's just not traverse the layer B-Tree while holding the layer map write lock. That's all I'm asking. If it can't be done now because it's difficult, add a TODO and issue for later.
Let's address the "reading 256MB under layer map write lock" problem, and leave the ctx issue for another day. I'll take out the context from the layer map later if necessary.
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.
Yes, it does. But it is stored in layer's inner. No need to pass it somewhere. Or may be I do not understand you.
It is not done under layer map lock. As I mentioned above it really done in = new_delta.get_holes(ctx)?
where no layer lock is hold.
No such problem. And moreover, traversing B-Tree doesn't mean reading all 256Mb. Size of layer index is about 40Mb
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.
Then why do we need
ctx
under write lock? (Rethorical question, hopefully makes it clear where the code readability problem is)Ok it's not an actual issue, just a readability issue. If it took 10 messages of back and forth to reach this conclusion, it deserves at least a comment in the code. I don't think it's at all obvious.
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.
Seems like: "
insert_historic
would usually read the layer to compute holes, but in practice that information would already be cached because the caller happens to callnew_delta.get_holes
."Is that the explanation? Or something else?
Even if that's the explanation, it's very fragile to line reordering refactors, and needs an extra comment..
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.
Sorry, looks like it is just some misunderstanding: I didn't understand your concerns and you didn't understand my explanations. As far as almost all reviewer complain about a lot of changes related with propagation of RequestContext and first of all concentrated on this issue I do not notice your main concern - that it can happen that we we perform some expensive operation (extracting holes) under layer map write lock. Actually it is not true, but it happens unintentionally - actually I didn't realize this problem. I will add comment explaining it. But I want to notice that this problem isn't somehow related with propagation of request contexts. It is present event ven if
DeltaLyaer:::load()
method doesn't require request context and I do not need to propagate it everywhere.