storage: fix bounds check in offset range size method #24880

andrwng · 2025-01-22T02:50:02Z

The methods to find offset range size currently use segment bounds to
determine whether a given offset is available to be queried. This
doesn't account for the case when the segment set contains offsets that
don't fall in the log's offset range (e.g. follow a delete records
request that trims mid-segment).

This commit adds appropriate bounds checks to both methods.

With an upcoming change to merge compact after windowed compaction,
test_offset_range_size2_compacted would fail because it would prefix
truncate mid-segment following a merge compaction, and then trip over
this, hitting an unexpected exception when creating a reader:

```
std::runtime_error: Reader cannot read before start of the log 0 < 887
```

Backports Required

Release Notes

None

The test itself has many steps that make it hard to follow what has happened. Adds some log messages to indicate different steps in the test.

Adds a static constructor to return an optional interval, which per the class comments, should represent an empty interval. I intend on using this for some upcoming bounds checks.

The methods to find offset range size currently use segment bounds to determine whether a given offset is available to be queried. This doesn't account for the case when the segment set contains offsets that don't fall in the log's offset range (e.g. follow a delete records request that trims mid-segment). This commit adds appropriate bounds checks to both methods. With an upcoming change to merge compact after windowed compaction, test_offset_range_size2_compacted would fail because it would prefix truncate mid-segment following a merge compaction, and then trip over this, hitting an unexpected exception when creating a reader: ``` std::runtime_error: Reader cannot read before start of the log 0 < 887 ```

vbotbuildovich · 2025-01-22T06:28:00Z

CI test results

test results on build#61022

test_id	test_kind	job_url	test_status	passed
rptest.tests.partition_balancer_test.PartitionBalancerTest.test_unavailable_nodes	ducktape	https://buildkite.com/redpanda/redpanda/builds/61022#01948c34-e8b6-4eb6-bc9f-eea76be5e1bc	FLAKY	1/2

Lazin

Didn't know about bounded_offset_interval. Looks good.

andrwng added 3 commits January 21, 2025 18:28

storage_e2e_test: add log messages indicating test progress

67689fa

The test itself has many steps that make it hard to follow what has happened. Adds some log messages to indicate different steps in the test.

model: add bounded_offset_interval::optional()

8514bfd

Adds a static constructor to return an optional interval, which per the class comments, should represent an empty interval. I intend on using this for some upcoming bounds checks.

github-actions bot added the area/redpanda label Jan 22, 2025

andrwng requested review from dotnwat, Lazin and nvartolomei January 22, 2025 02:50

andrwng mentioned this pull request Jan 22, 2025

storage: always schedule adjacent segment compaction #24874

Open

7 tasks

Lazin approved these changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: fix bounds check in offset range size method #24880

storage: fix bounds check in offset range size method #24880

andrwng commented Jan 22, 2025 •

edited

Loading

vbotbuildovich commented Jan 22, 2025

Lazin left a comment

storage: fix bounds check in offset range size method #24880

Are you sure you want to change the base?

storage: fix bounds check in offset range size method #24880

Conversation

andrwng commented Jan 22, 2025 • edited Loading

Backports Required

Release Notes

vbotbuildovich commented Jan 22, 2025

CI test results

Lazin left a comment

Choose a reason for hiding this comment

andrwng commented Jan 22, 2025 •

edited

Loading