-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
wal: use page buffered writer for writing records #6310
Conversation
Ok I will try this branch in failpoints cluster. |
@@ -36,7 +36,7 @@ type encoder struct { | |||
|
|||
func newEncoder(w io.Writer, prevCrc uint32) *encoder { | |||
return &encoder{ | |||
bw: bufio.NewWriter(w), | |||
bw: ioutil.NewPageWriter(w, 4096), |
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.
define 4096 somewhere?
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.
Also doc that this is buffering same as bufio https://github.com/golang/go/blob/master/src/bufio/bufio.go#L18?
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.
@gyuho it's only a coincidence the wal page size is the bufio default buffer size; the full buffer size for a pagewriter is 128k. I'll add a comment about the choice of the value.
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.
Got it. Thanks!
d54065f
to
91e7cdc
Compare
LGTM. Have you tried to run the benchmark in wal pkg to see if there is any performance gain? |
91e7cdc
to
d70d680
Compare
@xiang90 benchmark found a bug in the page writer 👍 Master:
This patch:
|
d70d680
to
5d3a9c1
Compare
A buffered writer that only writes full pages or when explicitly flushed.
Forces torn writes to only happen on sector boundaries. Fixes etcd-io#6271
5d3a9c1
to
28277b5
Compare
I can confirm that this fixes #6271 since it used to fail with less than 3 rounds. Now it's over 10 rounds. Thanks! |
lgtm |
Fixes #6271
/cc @gyuho