Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: Use fragmented_vector in OT-state #16642

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

Lazin
Copy link
Contributor

@Lazin Lazin commented Feb 19, 2024

offset_translator_state uses vector to store entries. This may create oversized allocations when the struct is serialized/deserialized. This commit replaces vector with chunked_vector.

Both serialized data structure and serialization code path are updated. The version of the data structure is not changed because serde treats any vector-like structure the same and uses the same format.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

Bug Fixes

  • Fix oversized allocation in storage.

offset_translator_state uses vector to store entries. This may create
oversized allocations when the struct is serialized/deserialized. This
commit replaces vector with chunked_vector.

Both serialized data structure and serialization code path are updated.
The version of the data structure is not changed because serde treats
any vector-like structure the same and uses the same format.
Copy link
Contributor

@nvartolomei nvartolomei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version of the data structure is not changed because serde treats any vector-like structure the same and uses the same format.

I trust you here. But, shouldn't we have a compatibility test for this struct anyway?

Do we have any integration tests that would catch this breaking during an upgrade? In the general case.

@piyushredpanda piyushredpanda merged commit 82e1270 into redpanda-data:dev Feb 20, 2024
17 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-16642-v23.3.x-169 remotes/upstream/v23.3.x
git cherry-pick -x ba038617d3f7e94ba1b14894f7c3fa3db598d852

Workflow run logs.

@dotnwat
Copy link
Member

dotnwat commented Feb 23, 2024

The version of the data structure is not changed because serde treats any vector-like structure the same and uses the same format.

I trust you here. But, shouldn't we have a compatibility test for this struct anyway?

Do we have any integration tests that would catch this breaking during an upgrade? In the general case.

@Lazin any thoughts here?

@Lazin
Copy link
Contributor Author

Lazin commented Feb 23, 2024

The version of the data structure is not changed because serde treats any vector-like structure the same and uses the same format.

I trust you here. But, shouldn't we have a compatibility test for this struct anyway?
Do we have any integration tests that would catch this breaking during an upgrade? In the general case.

@Lazin any thoughts here?

We're using the same code path for all vector-like structures in serde. They're all serialized using the same format.

@nvartolomei
Copy link
Contributor

nvartolomei commented Feb 23, 2024

My comment was guided by this https://vectorizedio.atlassian.net/wiki/spaces/CORE/pages/244252675/Serialization#Testing

Looking at the codebase I don't see binary blobs being tested from older versions against new definitions. I'm looking in the wrong place or we actually don't test that anywhere?

@dotnwat
Copy link
Member

dotnwat commented Feb 24, 2024

Looking at the codebase I don't see binary blobs being tested from older versions against new definitions. I'm looking in the wrong place or we actually don't test that anywhere?

there are some tests in the tree on a case-by-case basis, but we don't have a systematic framework for testing data across versions. we started this project a couple years ago, and made pretty good progress. we really should finish it!

at least in this pr, the element being serialized doesn't change, so i don't think there is a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants