instead of having constantly running thread, segment creator v2 spans…#98
instead of having constantly running thread, segment creator v2 spans…#98
Conversation
There was a problem hiding this comment.
The thread scheduling we have here seems a bit complicated to me. But, I don't see any problems with it. I like that we now create a thread on demand.
It looks like we can hit a 'no segments available' error if we'd request new segments very fast, where the segment creator doesn't have time to actually create the segment on disk. I'd rather block here to just wait on the segment being created to eliminate this edge case.
Left a few review remarks, mostly nitpicks.
| use std::path::{Path, PathBuf}; | ||
| use std::thread; | ||
|
|
||
| pub struct SegmentCreatorV2 { |
There was a problem hiding this comment.
Shall we keep it SegmentCreator, and properly replace the old one?
| segment_capacity: usize, | ||
| ) -> std::io::Result<Vec<OpenSegment>> { | ||
| // Directory being a file only applies to Linux | ||
| #[cfg(not(target_os = "windows"))] |
There was a problem hiding this comment.
Shall we make it a bit more specific, like
| #[cfg(not(target_os = "windows"))] | |
| #[cfg(target_os = "linux")] |
or
| #[cfg(not(target_os = "windows"))] | |
| #[cfg(target_family = "unix")] |
| } | ||
|
|
||
| #[cfg(not(target_os = "windows"))] | ||
| dir.sync_all()?; |
There was a problem hiding this comment.
Is this necessary? Why not just sync the segment files itself?
There was a problem hiding this comment.
I don't know, I just re-used existing logic
… threads on demand