Offset tracker invariants #17381

WillemKauf · 2024-03-25T23:19:33Z

Changed storage::segment::offset_tracker from a struct to a class, made data fields private.

Added public "getter" and "setter" functions in order to vassert the necessary offset invariants.

Also packaged in some very minor changes (see commit 85d979bd79f7a9e41cb83237673f483c25e02ac1), including

removal of superfluous partition_probe forward declaration in cloud_storage/fwd.h
correct usage of static constexpr variable in cluster/cloud_storage_size_reducer.h
Unnecessary virtual specifier on a member function of a class marked final in cluster/id_allocator.h

Backports Required

Release Notes

None

vbotbuildovich · 2024-03-26T01:28:13Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46740#018e7817-8b0f-460a-80a9-b5312b34a560

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46740#018e7823-f542-4138-8ffc-12448c07670a

dotnwat

looks good to me. i think a little commit history clean up is all that is needed.

src/v/cluster/cloud_storage_size_reducer.h

src/v/archival/archival_policy.cc

dotnwat · 2024-03-26T02:54:00Z

src/v/storage/segment.h

+            (set_offset_impl(std::forward<Ts>(ts)), ...);
+            vassert(
+		    _committed_offset <= _stable_offset && _stable_offset <= _dirty_offset,
+              "Must maintain offset invariant: committed <= stable <= dirty");


print out the values in the assertion failure to aid in debugging

dotnwat · 2024-03-26T02:55:30Z

src/v/storage/segment.h

+          , _dirty_offset(model::prev_offset(base)) {}
+
+        template<typename... Ts>
+        void set_offsets(Ts&&... ts) {


nit: this looks fine, but I think you can remove all the forwarding references and pass around T values. the model::offset types are just a single integer / trivially copyable.

Fair! Forwarding references changed to pass by value.

dotnwat · 2024-03-26T02:56:30Z

src/v/archival/archival_policy.cc

@@ -285,7 +285,9 @@ static ss::future<std::optional<std::error_code>> get_file_range(
    upl->final_file_offset = fsize;
    upl->base_timestamp = segment->index().base_timestamp();
    upl->max_timestamp = segment->index().max_timestamp();
-    if (!end_inclusive && segment->offsets().get_base_offset() == begin_inclusive) {


the two reformatting commits can be squashed together.

dotnwat · 2024-03-26T02:57:10Z

src/v/storage/segment.h

-                static_assert(sizeof(T) && false, "Invalid offset type");
+                static_assert(always_false_v<T>, "Invalid offset type");


can be squashed back into the original commit

dotnwat · 2024-03-26T02:57:19Z

src/v/storage/segment.h

-    //_tracker.committed_offset = _idx.max_offset();
-    //_tracker.stable_offset = _idx.max_offset();
-    //_tracker.dirty_offset = _idx.max_offset();


can be squashed back into the original commit

dotnwat · 2024-03-26T02:57:34Z

src/v/storage/tests/segment_offset_tracker_test.cc

+    storage_resources resources;
+};
+
+TEST_F(segment_offset_tracker_fixture, get_and_set_offsets) {


cluster: remove unnecessary virtual specifier cloud_storage: removal of extra forward declaration

dotnwat · 2024-03-26T14:57:28Z

src/v/storage/segment.h

+        model::term_id get_term() const { return _term; }
+        model::offset get_base_offset() const { return _base_offset; }
+        model::offset get_committed_offset() const { return _committed_offset; }
+        model::offset get_stable_offset() const { return _stable_offset; }
+        model::offset get_dirty_offset() const { return _dirty_offset; }


these accessors need to be in the previous commit so that the previous commit will compile.

nvartolomei · 2024-03-27T10:25:48Z

src/v/storage/segment.h

+            set_offsets(t);
+        }
+
+        model::term_id get_term() const { return _term; }


nit: get_ prefix is just noise to me. I suggest we name them just term(), base_offest() in the future. @dotnwat wdyt?

@nvartolomei yes, I think get_ is too verbose. If we want a prefix, I think using it for set_ is better as it is generally used far less.

Maybe bring it up in core storage and we can agree on a style and then change this depending on the outcome?

nvartolomei · 2024-03-27T10:26:10Z

src/v/storage/segment.h

+          , _dirty_offset(model::prev_offset(base)) {}
+
+        template<typename... Ts>
+        void set_offsets(Ts... ts) {


Neat & clever! 👍

Shall we also have a mean of enforcing that offsets don't move backwards? Unless it is a call to truncate?

WillemKauf requested a review from dotnwat March 25, 2024 23:19

github-actions bot added the area/redpanda label Mar 25, 2024

piyushredpanda requested a review from nvartolomei March 26, 2024 02:04

dotnwat reviewed Mar 26, 2024

View reviewed changes

cluster: proper use of named constant

8f55e6e

cluster: remove unnecessary virtual specifier cloud_storage: removal of extra forward declaration

WillemKauf force-pushed the offset_tracker_invariants branch from 808a3c4 to ef97c40 Compare March 26, 2024 12:55

dotnwat reviewed Mar 26, 2024

View reviewed changes

WillemKauf added 4 commits March 26, 2024 11:28

storage::segment::offset_tracker access renames

fe2df0d

storage::segment::offset_tracker class changes

e9a4ae9

offset_tracker unit test

dd9fc00

clang-format

a58bc90

WillemKauf force-pushed the offset_tracker_invariants branch from ef97c40 to a58bc90 Compare March 26, 2024 15:44

dotnwat approved these changes Mar 26, 2024

View reviewed changes

dotnwat merged commit 908d9dc into redpanda-data:dev Mar 26, 2024
17 checks passed

nvartolomei reviewed Mar 27, 2024

View reviewed changes

WillemKauf deleted the offset_tracker_invariants branch April 13, 2024 03:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offset tracker invariants #17381

Offset tracker invariants #17381

WillemKauf commented Mar 25, 2024 •

edited

vbotbuildovich commented Mar 26, 2024 •

edited

dotnwat left a comment

dotnwat Mar 26, 2024

dotnwat Mar 26, 2024

WillemKauf Mar 26, 2024

dotnwat Mar 26, 2024

dotnwat Mar 26, 2024

dotnwat Mar 26, 2024

dotnwat Mar 26, 2024

dotnwat Mar 26, 2024

nvartolomei Mar 27, 2024

dotnwat Mar 27, 2024

nvartolomei Mar 27, 2024

nvartolomei Mar 27, 2024

		static_assert(sizeof(T) && false, "Invalid offset type");
		static_assert(always_false_v<T>, "Invalid offset type");

Offset tracker invariants #17381

Offset tracker invariants #17381

Conversation

WillemKauf commented Mar 25, 2024 • edited

Backports Required

Release Notes

vbotbuildovich commented Mar 26, 2024 • edited

dotnwat left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf commented Mar 25, 2024 •

edited

vbotbuildovich commented Mar 26, 2024 •

edited