Conversation
5be91d5 to
28d48c1
Compare
30d264d to
18b2aa6
Compare
|
This branch is Crasher approved ✔️ |
995bf4f to
239e6c6
Compare
The snapshot operation explicitly flushes segments. We therefore must satisfy flush ordering there too.
26dc545 to
20f4562
Compare
| @@ -62,13 +62,8 @@ | |||
| .collect(); | |||
| segment_reads.reverse(); | |||
There was a problem hiding this comment.
Do we need this reverse if we do sort_* right after?
| /// If there are unsaved changes after flush - detects lowest unsaved change version. | ||
| /// If all changes are saved - returns max version. | ||
| pub fn flush_all(&self, sync: bool, force: bool) -> OperationResult<SeqNumberType> { | ||
| let lock_order: Vec<_> = self.segment_flush_ordering().collect(); |
There was a problem hiding this comment.
Also, do we need to adjust this as well? Open question, cause I'm not sure if we do. But it's a bit confusing that we have segment_flush_ordering method and SegmentFlushOrdering type, and they are not related (e.g., one does not depend on the other).
There was a problem hiding this comment.
I agree, this is confusing.
Can we unify them?
There was a problem hiding this comment.
I agree we must unify this. I have some changes for the segment holder in mind, and will do that in a bit.
|
Overall, the idea of PR makes sense IMO, but I'm confused by |
Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
agourlay
left a comment
There was a problem hiding this comment.
Approving to let it bake on our mean CIs.
Expecting the mentioned unification to be part of a next PR.
|
I agree the current design isn't great as noted in #7381 (comment). Though, we do agree that a change like this makes sense. Even though a better design is pending, I'll now merge this anyway. It gives us a bit more time to test the change in practice on our chaos testing setup. I have a better design approach in mind which I'll implement asynchronously to still meet with the review remarks. I'll not change behavior and so merging this should be fine. Then we can discuss the design changes in more detail in a dedicated PR. Update: initial design improvements handled in #7436 |
* Re-sort segments on flush, don't use proxy but inner segment state * Also sort segments when proxying all segments for snapshot The snapshot operation explicitly flushes segments. We therefore must satisfy flush ordering there too. * Add some helpful comments * Add explicit flush ordering enum for segments * Minor tweaks * Fix spelling in comment Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com> --------- Co-authored-by: Arnaud Gourlay <arnaud.gourlay@gmail.com>
Depends on #7388
Improves flush ordering for segments. The main problem is proxy segments shadowing appendable state of the actual segments.
We have a strict ordering requirement for segments when flushing to retain data consistency. We must flush appendable segments first.
Proxy segments are always considered non-appendable since #7345, even though the wrapped segment might originally have been appendable.
In case of flushing, we care about the wrapped segments. They hold the physical data and might have changes pending flush. A proxy wrapping an appendable segment must still always be flushed before any non-appendable segments.
The proxy segments shadowed the appendable state, which messed with the flush ordering. To solve this, I now define desired flush ordering for each segment with a state. Proxy segments are flushed in between regular appendable and non-appendable segments.
As a secondary change this also enforces the same ordering for proxying all segments during snapshot. Snapshots now explicitly flush segments, and so it must follow the same ordering requirements.
Tasks
devAll Submissions:
devbranch. Did you create your branch fromdev?