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

[FIXED] Check expected record size before loading the payload #1266

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Jul 29, 2022

Reverted addition of record_size_limit (#1259, #1260, #1262)

But still address the memory usage caused by a corrupted data message
on recovery.

By using the expected record size from the index file, when checking
that the last message matches the index information, we would find
out that the index's stored message record size does not match the
record size in the ".dat" file and would not allocate the memory
to read the rest of the message.

The record_size_limit that was added to solve that issue would have
likely caused a lot of issues if mis-used.

Resolves #1255

Signed-off-by: Ivan Kozlovic ivan@synadia.com

Reverted addition of record_size_limit

But still address the memory usage caused by a corrupted data message
on recovery.

By using the expected record size from the index file, when checking
that the last message matches the index information, we would find
out that the index's stored message record size does not match the
record size in the ".dat" file and would not allocate the memory
to read the rest of the message.

The record_size_limit that was added to solve that issue would have
likely caused a lot of issues if mis-used.

Resolves #1255

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic kozlovic requested a review from aricart July 29, 2022 22:43
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 91.395% when pulling 3983687 on revert_record_size_limit into 2f0c582 on main.

server/conf.go Show resolved Hide resolved
Copy link
Member

@aricart aricart left a comment

Choose a reason for hiding this comment

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

LGTM

@kozlovic kozlovic merged commit 7ceff6c into main Aug 1, 2022
@kozlovic kozlovic deleted the revert_record_size_limit branch August 1, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

High memory consumption while recovering damaged data file (in scenario in-file storage)
3 participants