Phil/pruning is hard #362
Phil/pruning is hard #362
Conversation
This reverts commit e2cace3. Unfortunately, this did not actually work in practice to allow shard pruning to work based on the combined set of backup and primary hints. Trying a new thing in the next commit.
psFried
force-pushed
the
phil/pruning-is-hard
branch
2 times, most recently
from
bb00810
to
07c98c9
Compare
cmd/gazctl/gazctlcmd/shards_prune.go
Outdated
| @@ -101,7 +101,11 @@ func (cmd *cmdShardsPrune) Execute([]string) error { | |||
| metrics.fragmentsTotal++ | |||
| metrics.bytesTotal += spec.ContentLength() | |||
|
|
|||
| if len(segments.Intersect(journal, spec.Begin, spec.End)) == 0 { | |||
| // belt-with-suspenders, to ensure we never try to prune un-persisted fragments. | |||
| if spec.End == 0 { | |||
There was a problem hiding this comment.
All fragments -- even un-persisted ones -- are always placed within the journal (have a Begin and End).
AFAIK it's not even possible for brokers to return a fragment with End == 0
What lead to this check?
There was a problem hiding this comment.
What lead to this check?
Just a bit of paranoia, and feeling like the zeroing of LastOffset in the last segment seemed a bit tricky. I'll remove it
| @@ -123,6 +127,16 @@ func (cmd *cmdShardsPrune) Execute([]string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func overlapsAnySegment(segments []recoverylog.Segment, fragment pb.Fragment) bool { | |||
| for _, seg := range segments { | |||
| if (seg.FirstOffset < fragment.End || fragment.End == 0) && | |||
There was a problem hiding this comment.
I'd expect the fragment.End check can be dropped per above.
Otherwise, I ran through a bunch of mental exercises proving to myself this otherwise handles all of the possible overlap case. I presume you did the same. I couldn't come up with any cases where it misbehaves.
There was a problem hiding this comment.
I presume you did the same.
Yeah, and also updated the tests to use this function so that we're exercising it. I waffled a bit on whether to add unit tests for just this function, and would be happy to do so if you think it'd help
…tely When pruning recovery logs, keep a slice of all the segments from each set of hints. Then check each segment separately, pruning only fragments that are not overlapped by any of them. This works around difficulties we've had in the past with merging segments from different hints into a single `SegmentSet` (which is not what it was designed for).
psFried
force-pushed
the
phil/pruning-is-hard
branch
from
07c98c9
to
7c8723e
Compare
This reverts the previous attempt to fix recovery log pruning, and replaces it with a slightly different approach. Individual commit messages have more details, but the gist is to avoid trying to merge segment sets from different hints, and instead just check each set of hints separately.
One question I have is how to handle recovery logs that are used by multiple shards (that have since split). For example, say we have shards
a/0anda/1, which both have hints coveringa/log. As of this PR, we would still potentially be merging the hints from multiple shards into a single set, since the maps are keyed on journal name and hints name, and both shards will have hints with the same name. So I'm thinking it may be safer to key the hints on journal name and a combined shardId+hintsName (for example"a/log": { "a/0-primary": <hints>, "a/1-primary": <hints>, ...}). But I'm not really sure whether that's necessary, so curious to get your thoughts.This change is