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.

Sign up for GitHub

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

Jump to bottom

Handle empty msg files on start #685

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Handle empty msg files on start #685

wants to merge 12 commits into from
+71 −1

Conversation

viktorerlingsson
Copy link
Member

WHAT is this pull request doing?

Handles empty msg files on startup and sets Schema version to default.

I think this can happen if a queue is created and lavinmq is killed before anything is written to the file.

Fixes #624

HOW can this pull request be tested?

Run specs

@viktorerlingsson viktorerlingsson changed the title 624 handle empty msg files on start Handle empty msg files on start May 22, 2024
spuun
spuun reviewed May 24, 2024
View reviewed changes
Copy link
Member

@spuun spuun left a comment

Choose a reason for hiding this comment

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

Interesting that this is happening. Is it because an ungraceful shutdown won't truncate the file?

src/lavinmq/queue/message_store.cr Outdated
file = file.not_nil!
file.resize(0)
file.write_bytes Schema::VERSION
@replicator.not_nil!.try &.append path.not_nil!, Schema::VERSION
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@replicator.not_nil!.try &.append path.not_nil!, Schema::VERSION
@replicator.try &.append path.not_nil!, Schema::VERSION

I think you can avoid the other two not_nil! calls by moving the rescue to a block around SchemaVersion:

begin
  SchemaVersion.verify(file, :message)
rescue EmptyFile
  Log.warn { "Empty file at #{path}, setting schema version to #{Schema::VERSION}" }
  file.resize(0)
  file.write_bytes Schema::VERSION
  @replicator.not_nil!.try &.append path, Schema::VERSION
end

I'm also wondering how replication will handle this? If the file has a size, append will write to the wrong position in follower? Maybe it needs to be

  @replicator.try &.append file, 0, sizeof(Int32)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you can avoid the other two not_nil! calls by moving the rescue to a block around SchemaVersion:

Thanks, that's much better 👍

@replicator.try &.append path, file, 0, sizeof(Int32))
Should pos be 0 here?

Do we need to do both @replicator.try &.append path, file, 0, sizeof(Int32) and @replicator.try &.append path, Schema::VERSION?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be solved now when we delete & recreate files, right?

src/lavinmq/schema.cr Outdated Show resolved Hide resolved
@viktorerlingsson
Copy link
Member Author

Interesting that this is happening. Is it because an ungraceful shutdown won't truncate the file?

Yes, but I think it has to happen before an fsync as well, because otherwise we would write schema version when opening a segment?

@carlhoerberg
Copy link
Member

Yes, would be good to understand the root cause of this, although i agree with the fix, if first 16 bytes are 0 most likely the rest is 0 too. But we always write schema version to the mmap when a new segment is opened, even if lavinmq crashed exactly after that it would still be written to disk (changes to the mmap is persisted on unmap/crash), only if there was a powerloss or disk failure would it not be written to disk. Or lavin crashed before writing the schema version, but there's like nanoseconds between the file was created and the schema version is written.

lavinmq/src/lavinmq/queue/message_store.cr

Lines 248 to 249 in 136b743

wfile = MFile.new(path, capacity)
wfile.write_bytes Schema::VERSION

@viktorerlingsson viktorerlingsson force-pushed the 624-handle_empty_msg_files_on_start branch from 9baaad0 to 6b04b14 Compare June 19, 2024 15:37
carlhoerberg
carlhoerberg reviewed Jun 19, 2024
View reviewed changes
spec/schema_version_spec.cr Outdated Show resolved Hide resolved
spec/schema_version_spec.cr Outdated Show resolved Hide resolved
spec/schema_version_spec.cr Outdated Show resolved Hide resolved
spec/schema_version_spec.cr Outdated Show resolved Hide resolved
src/lavinmq/schema.cr Outdated Show resolved Hide resolved
src/lavinmq/queue/message_store.cr Outdated Show resolved Hide resolved
@viktorerlingsson viktorerlingsson mentioned this pull request Jun 20, 2024
Open
carlhoerberg
carlhoerberg reviewed Jun 20, 2024
View reviewed changes
src/lavinmq/schema.cr Outdated Show resolved Hide resolved
@viktorerlingsson @carlhoerberg
Raise EOFError instead of EmptyFile. Use read_at instead of file.to_s…
2522900 
…lice. Simpler zero check.

Co-authored-by: Carl Hörberg <[email protected]>
@viktorerlingsson viktorerlingsson force-pushed the 624-handle_empty_msg_files_on_start branch from e9c70d3 to 2522900 Compare June 20, 2024 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlhoerberg carlhoerberg carlhoerberg left review comments

@kickster97 kickster97 Awaiting requested review from kickster97 kickster97 is a code owner

@snichme snichme Awaiting requested review from snichme snichme is a code owner

@spuun spuun Awaiting requested review from spuun spuun is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@viktorerlingsson @carlhoerberg @spuun