-
Notifications
You must be signed in to change notification settings - Fork 32
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
Handle empty msg files on start #685
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
Yes, but I think it has to happen before an fsync as well, because otherwise we would write schema version when opening a segment? |
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 lavinmq/src/lavinmq/queue/message_store.cr Lines 248 to 249 in 136b743
|
9baaad0
to
6b04b14
Compare
Co-authored-by: Carl Hörberg <[email protected]>
Co-authored-by: Carl Hörberg <[email protected]>
…lice. Simpler zero check. Co-authored-by: Carl Hörberg <[email protected]>
e9c70d3
to
2522900
Compare
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