-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
binaryHeader: Fixed partial write issue for index-header. #2371
Conversation
cc @mkabischev |
|
||
// Buffer for copying and encbuffers. | ||
// This also will control the size of file writer buffer. | ||
buf := make([]byte, 32*1024) | ||
bw, err := newBinaryWriter(fn, buf) | ||
bw, err := newBinaryWriter(tmpFilename+".tmp", buf) |
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.
.tmp
added twice?
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.
Fixed. 🤦♂️
Fixes #2213 This caused was indicated as regression of latency, and also causes potential critical issue for store GW, where manual delete of index-header from local storage was required. This might be considered as blocker for 0.12, so it would be worth to port it to 0.12 TBH @squat. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
if err != nil { | ||
return errors.Wrap(err, "new binary index header writer") | ||
} | ||
defer runutil.CloseWithErrCapture(&err, bw, "close binary writer for %s", fn) | ||
defer runutil.CloseWithErrCapture(&err, bw, "close binary writer for %s", tmpFilename) |
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.
Since binaryWriter.Close()
calls FileWriter.Close()
which does file.Sync()
and file.Flush()
maybe it would be worth being extra careful and explicitly calling it before os.Rename
?
Or we could do another round of:
if err := bw.f.Flush(); err != nil {
return errors.Wrap(err, "flush")
}
& Sync()
. WDYT?
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.
yes!
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.
Thanks for this good point
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.
lgtm
if we want this in v0.12.0, should we instead PR into release-0.12? |
Yes, will do with @GiedriusS fixes. |
Correct me if I'm wrong, but I think we can close this as we're going to be merging this into master when we merge |
ack we can close this now @brancz 👍 |
Fixes #2213
This cause was indicated as the potential root cause for regression of latency and also causes the potentially critical issue
for store GW, where manual delete of index-header from local storage was required.
This might be considered as a blocker for 0.12, so it would be worth it to port it to 0.12 TBH @squat WDYT?
Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com