mvlog: log basics for segment rolls #18521

andrwng · 2024-05-16T07:56:34Z

Introduces the beginnings of a log implementation that is able to roll
new segments with segment.bytes or segment.ms.

The log, similar to the disk_log_impl, is backed by a circular buffer of
segments, with the noteable difference that the active segment is
managed explicitly and separately from the others.

I'm not fully sold on this approach (e.g. vs only managing the segment
appender separately but keeping the underlying readable segment with the
rest of the segments), but for now it makes reasoning about segment
rolling concurrency a bit more straightforward, and down the line it
will make it easier to reason about segment removals (e.g. the active
segment cannot be removed).

This implementation is still missing quite a bit (e.g. it doesn't flush,
doesn't implement the exact log interface), but at least begins to
introduce some ideas that can back the new log implementation.

Backports Required

Release Notes

none

andrwng · 2024-05-16T15:16:52Z

/ci-repeat

vbotbuildovich · 2024-05-16T20:14:17Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49240#018f82d0-6c12-4eb4-912b-8ba00e7a9aa8

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49255#018f83cd-a3a8-4ab2-af72-f7b390239ff8

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49388#018f9d29-bd08-4032-844c-caafabf055fb

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/49438#018fa139-a10b-4a06-bb8c-5f0d63bd5c92

This will be used to uniquely identify new segments.

src/v/storage/mvlog/CMakeLists.txt

src/v/storage/mvlog/tests/active_segment_test.cc

dotnwat · 2024-05-21T00:45:06Z

src/v/storage/mvlog/versioned_log.cc

+  : segment_file(std::move(f))
+  , appender(std::make_unique<segment_appender>(segment_file.get()))
+  , readable_seg(std::make_unique<readable_segment>(segment_file.get()))
+  , construct_time(ss::lowres_clock::now())


would construction time be attached to the file instead?

Do you mean whether construction time should be something tracked in the mvlog::file? I think probably not -- the construction time is only really useful for now in determining if we should apply segment.ms

Or are you asking whether it should be attached to something other than the file construction (e.g. first data write)? I think that wouldn't be a bad idea -- maybe it'd be important to ensure no "empty" segments e.g. if a segment contains only truncate entries maybe we wouldn't want it being rolled? Though I'm not sold that such empty segments are a problem

I was thinking that ctime is a normal posix file property, so associating it with the actual file made some sense. but maybe it's just a naming collision? we probably want to track our own creation timestamp in mvlog if that timestamp is going to be used for retention purposes.

Ah gotcha. Yeah it probably makes sense for retention. Will ponder a bit on how to resolve the naming conflict. I agree it might be confusing down the line

src/v/storage/mvlog/versioned_log.cc

dotnwat · 2024-05-21T00:51:31Z

src/v/storage/mvlog/versioned_log.cc

+    auto ro_seg = std::make_unique<readonly_segment>(std::move(active_seg_));
+    segs_.emplace_back(std::move(ro_seg));


cool. i wonder if we should call this like extend rather than roll? i mean, it doesn't really matter, but I am reading correctly that this roll is not like the roll we do in disk_log_impl--it's a logical roll?

Hm, a relatively self-describing name might be close_active_seg_unlocked() or something. You're right in disk_log_impl "roll" typically refers to creating an additional segment, which this doesn't do.

I'm not a huge fan of extend because it might be mistaken for adding data to the log, which this isn't doing, unless you're referring to the create_unlocked() method?

got it. i think it's fine as-is. i'm getting used to the naming

dotnwat · 2024-05-21T00:52:09Z

src/v/storage/mvlog/versioned_log.cc

+    vassert(
+      !active_segment_lock_.ready(),
+      "create_unlocked() must be called with active segment lock held");
+    vassert(active_seg_ == nullptr, "Expected no active segment");


maybe there isn't, but it would be nice if this were impossible by construction. sort of like, a log always has an active segment--it's the last one?

Yeah I agree, though I haven't landed yet on an impl that I found easy to reason about. It's possible that everything covered by the active segment lock could be encapsulated into some active_segment_manager class that handles rolling and appends and such. I suspect it'll make things harder to reason about on the read path, but I wasn't sure so I opted to keep this a little more raw

dotnwat · 2024-05-21T00:57:05Z

src/v/storage/mvlog/versioned_log.cc

+      log.info,
+      "Rolling segment file {}",
+      active_seg_->segment_file->filepath().c_str());
+    co_await active_seg_->segment_file->flush();


i wonder if the flush is necessary? if we extend the log and keep writing to the next segment at some point the "log" will be flushed, at which point we could flush all unflushed segments. is there a particular reason for flushing here before rolling?

Good point, I felt it seemed natural for applying segment.ms for a flush to occur because it indicates we have some data that's otherwise just sitting in memory. But for segment.bytes maybe it's more worth it to be lazy about flushing

yeh. we can always measure it and see if it is worth keeping or removing.

src/v/storage/mvlog/versioned_log.cc

src/v/storage/mvlog/versioned_log.h

Lazin · 2024-05-21T13:06:24Z

Generally, the change looks good. I'm curious about the versioning. Previously, we had a lot of complexity created by use of locking. This code also relies on locking instead of versioning. Or maybe versioning will be used for truncation/readers?

andrwng · 2024-05-21T20:50:01Z

Previously, we had a lot of complexity created by use of locking. This code also relies on locking instead of versioning. Or maybe versioning will be used for truncation/readers?

Good observation. I think the complexity of segment locking is that it allows for there to be many combinations of orderings of lock acquisitions, which makes it very easy to deadlock. I'm hoping that by more narrowly scoping the areas that we have locks that we'll reduce complexity.

And that's right, we'll use versioning on the read path and in handling truncations.

nvartolomei · 2024-05-22T13:25:01Z

src/v/storage/mvlog/versioned_log.cc

+}
+
+ss::future<> versioned_log::apply_segment_ms() {
+    auto lock = active_segment_lock_.get_units();


missing co_await? i thought this would fail 🤔 or we never added this diagnostic? llvm/llvm-project#76101

Yikes! Fixed

Added a test for this as well.

WillemKauf · 2024-05-23T16:49:36Z

src/v/storage/mvlog/versioned_log.cc

+      "Rolling segment file {}",
+      active_seg_->segment_file->filepath().c_str());
+    co_await active_seg_->segment_file->flush();
+    auto ro_seg = std::make_unique<readonly_segment>(std::move(active_seg_));


nit: Is auto ro_seg = std::move(active_seg_) on its own sufficient?

The active segment and readonly segments have different types, so it doesn't seem like it

Oops. Eyes deceived me on this one.

src/v/storage/mvlog/versioned_log.cc

WillemKauf · 2024-05-23T16:54:21Z

src/v/storage/mvlog/tests/active_segment_test.cc

+        return log_.get();
+    }
+
+    ss::future<ss::circular_buffer<model::record_batch>>


nit: For future proofing, maybe this should use the version_log::segments_t type (you'll have to make it public).

This is a group of batches, not segments

src/v/storage/mvlog/versioned_log.cc

src/v/storage/mvlog/versioned_log.h

Introduces the beginnings of a log implementation that is able to roll new segments with segment.bytes or segment.ms. The log, similar to the disk_log_impl, is backed by a circular buffer of segments, with the noteable difference that the active segment is managed explicitly and separately from the others. I'm not fully sold on this approach (e.g. vs only managing the segment appender separately but keeping the underlying readable segment with the rest of the segments), but for now it makes reasoning about segment rolling concurrency a bit more straightforward, and down the line it will make it easier to reason about segment removals (e.g. the active segment cannot be removed). This implementation is still missing quite a bit (e.g. it doesn't flush, doesn't implement the exact log interface), but at least begins to introduce some ideas that can back the new log implementation.

github-actions bot added the area/redpanda label May 16, 2024

andrwng force-pushed the mvlog-segment-rolls branch from 8858a98 to e81cc70 Compare May 16, 2024 08:01

andrwng force-pushed the mvlog-segment-rolls branch 2 times, most recently from bc6a94c to 4651b15 Compare May 16, 2024 17:37

mvlog: add named type for segment id

082f652

This will be used to uniquely identify new segments.

andrwng force-pushed the mvlog-segment-rolls branch from 4651b15 to 92c6b5f Compare May 16, 2024 22:42

andrwng requested review from dotnwat, Lazin and WillemKauf May 16, 2024 22:42

andrwng mentioned this pull request May 17, 2024

mvlog: add gaps to the segment reader #18570

Merged

7 tasks

andrwng requested a review from nvartolomei May 18, 2024 02:03

andrwng assigned dotnwat and nvartolomei May 21, 2024

dotnwat previously approved these changes May 21, 2024

View reviewed changes

nvartolomei reviewed May 21, 2024

View reviewed changes

src/v/storage/mvlog/versioned_log.cc Outdated Show resolved Hide resolved

src/v/storage/mvlog/versioned_log.cc Show resolved Hide resolved

Lazin reviewed May 21, 2024

View reviewed changes

src/v/storage/mvlog/versioned_log.cc Outdated Show resolved Hide resolved

src/v/storage/mvlog/versioned_log.h Show resolved Hide resolved

andrwng dismissed dotnwat’s stale review via ce88be0 May 21, 2024 20:50

andrwng force-pushed the mvlog-segment-rolls branch from 92c6b5f to ce88be0 Compare May 21, 2024 20:50

andrwng requested review from dotnwat, nvartolomei and Lazin May 21, 2024 20:58

nvartolomei reviewed May 22, 2024

View reviewed changes

andrwng force-pushed the mvlog-segment-rolls branch 2 times, most recently from d27b749 to 8b291b3 Compare May 22, 2024 20:05

andrwng requested a review from nvartolomei May 23, 2024 00:36

WillemKauf reviewed May 23, 2024

View reviewed changes

src/v/storage/mvlog/versioned_log.cc Outdated Show resolved Hide resolved

WillemKauf reviewed May 23, 2024

View reviewed changes

src/v/storage/mvlog/versioned_log.cc Outdated Show resolved Hide resolved

WillemKauf reviewed May 23, 2024

View reviewed changes

src/v/storage/mvlog/versioned_log.cc Outdated Show resolved Hide resolved

WillemKauf reviewed May 23, 2024

View reviewed changes

src/v/storage/mvlog/versioned_log.h Outdated Show resolved Hide resolved

andrwng force-pushed the mvlog-segment-rolls branch from 8b291b3 to 22e39f0 Compare May 23, 2024 19:39

andrwng requested a review from WillemKauf May 23, 2024 23:11

WillemKauf approved these changes May 24, 2024

View reviewed changes

dotnwat approved these changes May 24, 2024

View reviewed changes

andrwng merged commit 122097b into redpanda-data:dev May 24, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mvlog: log basics for segment rolls #18521

mvlog: log basics for segment rolls #18521

andrwng commented May 16, 2024 •

edited

andrwng commented May 16, 2024

vbotbuildovich commented May 16, 2024 •

edited

dotnwat May 21, 2024

andrwng May 21, 2024

dotnwat May 24, 2024 •

edited

andrwng May 24, 2024

dotnwat May 21, 2024

andrwng May 21, 2024

andrwng May 21, 2024

dotnwat May 24, 2024

dotnwat May 21, 2024

andrwng May 21, 2024

dotnwat May 24, 2024

dotnwat May 21, 2024

andrwng May 21, 2024

dotnwat May 24, 2024

Lazin commented May 21, 2024

andrwng commented May 21, 2024

nvartolomei May 22, 2024

andrwng May 22, 2024

andrwng May 22, 2024

WillemKauf May 23, 2024 •

edited

andrwng May 23, 2024

WillemKauf May 24, 2024

WillemKauf May 23, 2024

andrwng May 23, 2024

		auto ro_seg = std::make_unique<readonly_segment>(std::move(active_seg_));
		segs_.emplace_back(std::move(ro_seg));

mvlog: log basics for segment rolls #18521

mvlog: log basics for segment rolls #18521

Conversation

andrwng commented May 16, 2024 • edited

Backports Required

Release Notes

andrwng commented May 16, 2024

vbotbuildovich commented May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnwat May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lazin commented May 21, 2024

andrwng commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrwng commented May 16, 2024 •

edited

vbotbuildovich commented May 16, 2024 •

edited

dotnwat May 24, 2024 •

edited

WillemKauf May 23, 2024 •

edited