mysql: improve code structure for buffered vs unbuffered writes #4190

sougou · 2018-09-09T04:20:21Z

@danieltahara @LK4D4: can you take a look at this approach?

The previous code had context sensitive functions that could not be freely reused because some assumed that they were using bufio and some didn't. This is now changed in such a way that the
buffering decision is made at a high level (only for ComQuery results).

This also changes how we would pool bufio.Writer compared to #4188.

Signed-off-by: Sugu Sougoumarane ssougou@gmail.com

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

go/mysql/server.go

-					return io.EOF
-				}
+			success := func() bool {
+				c.startBuffering()


danieltahara · 2018-09-09T07:31:38Z

go/mysql/conn.go

-		// With direct=true, the caller expects a flush, so we call it
-		// manually.
-		if err := c.writePacket(*c.currentEphemeralWriteBuffer); err != nil {
+		if err := c.writePacket(w, *c.currentEphemeralWriteBuffer); err != nil {


perhaps make this not a function on conn anymore?

ah. we need sequence number. that said, if we take suggestion above of swapping out writer at a higher level (when we start buffering), then we no longer need to pass the writer in as an argument to writePacket

getWriter should address this concern also.

danieltahara · 2018-09-09T07:34:48Z

go/mysql/conn.go

-	if direct {
-		w = c.conn
+	var w io.Writer = c.conn
+	if c.writer != nil {


i'd almost like to see this swapout happen with startBuffering call, and then instead of calling flush directly, startBuffering returns a finisher function func() that resets c.conn to the direct connectin appropriately and then calls flush.

This seems like a lot of trickiness, but I think what I'm trying to go for is to completely abstract away whether or not the connection is buffered or not from the writeXXX functions. @LK4D4 initially did this by pushing the buffering down into the writer object, and what you're doinig here is actually pulling the buffering control UP into the caller. i think the advantages of making flush easier to reason about and see make this approach worthwhile, in which case i'd like to go the rest of the way and get rid of the buffered writer juggling down here.

c.writer being nil also acts as a state to indicate that we're buffering. If we hide that, we'll need a different variable to track the state.

But I can wrap this inside a getWriter function which will remove the awkwardness of writePacket; It can also call it at the beginning.

As for the finisher function, c.flush is that. I don't see much value in returning it as is for startBuffering. Finishers make more sense if they did different things or acted on different objects, like in the case of contexts.

danieltahara · 2018-09-09T17:35:33Z

I think getWriter hits what I'm after :) -- Daniel dktahara@gmail.com (646) 397-6379

sougou · 2018-09-09T17:54:20Z

This is ready for review.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

go/mysql/conn.go

danieltahara · 2018-09-09T20:33:39Z

go/mysql/conn.go

 	switch c.currentEphemeralPolicy {
 	case ephemeralWriteSingleBuffer:
 		// Write the allocated buffer as a single buffer.
 		// It has both header and data.
-		if n, err := w.Write(*c.currentEphemeralWriteBuffer); err != nil {
+		if n, err := c.getWriter().Write(*c.currentEphemeralWriteBuffer); err != nil {
 			return fmt.Errorf("Conn %v: Write(*c.currentEphemeralWriteBuffer) failed: %v", c.ID(), err)


do you remember why there was no flush() path here?

If direct is true, then we directly write to c.conn. So, there's no need to flush. There is a subtle futuristic bug: if someone had previously invoked writeEphemeral with false, then something would be sitting in the write buffer, and the next call with true would skip those buffered items, and directly write the current packet to the conn.

The new code protects from this possibility.

hrmm, then why was there one on line 493?

That one was there because the code decides to use bufio in spite of the direct flag. So, it had to flush to maintain the contract. You'll see that in the old code, writePacket always used c.writer without regard to the direct flag.

ah okay. thanks for explanation

go/mysql/server.go

Handling error from flush required me to pull it out of the wrapped function, and this ended up making it unnecessary. The flush is now called at the end of the block. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

danieltahara

This looks good to me. If @LK4D4 could do a pass as well before landing, that'd be great.

danieltahara · 2018-09-10T05:01:40Z

go/mysql/conn.go

 	switch c.currentEphemeralPolicy {
 	case ephemeralWriteSingleBuffer:
 		// Write the allocated buffer as a single buffer.
 		// It has both header and data.
-		if n, err := w.Write(*c.currentEphemeralWriteBuffer); err != nil {
+		if n, err := c.getWriter().Write(*c.currentEphemeralWriteBuffer); err != nil {
 			return fmt.Errorf("Conn %v: Write(*c.currentEphemeralWriteBuffer) failed: %v", c.ID(), err)


ah okay. thanks for explanation

LK4D4 · 2018-09-10T16:02:36Z

go/mysql/conn.go

 	}
+
+	defer func() {


I think defer here adds to the complexity.
Can we just

err := c.bufferedWriter.Flush() ... return err

The defer protects us from accidental panics. It's the recommended pattern unless there is a specific reason like performance, etc.

go/mysql/server.go

If there are write errors while writing responses, then the handle function may return without flush getting called. This change calls flush on the defer to make sure we return the write buffer back to the pool. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

LK4D4 · 2018-09-10T21:03:07Z

@sougou LGTM

sougou · 2018-09-11T00:30:52Z

For some reason, the build result is not feeding back into the checks eventhough it's green. I'm going to force merge this.

Proposed alternate way to use write buffer.

3309b4f

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

danieltahara reviewed Sep 9, 2018

View reviewed changes

sougou changed the title ~~Proposed alternate way to use write buffer.~~ mysql: improve code structure for buffered vs unbuffered writes Sep 9, 2018

sougou force-pushed the mysql branch 2 times, most recently from 9076e74 to 6e92bd6 Compare September 9, 2018 18:04

mysql: address review comments and fix tests

b97ba65

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

sougou force-pushed the mysql branch from 6e92bd6 to b97ba65 Compare September 9, 2018 18:12

danieltahara suggested changes Sep 9, 2018

View reviewed changes

mysql: address further review comments.

0f34322

Handling error from flush required me to pull it out of the wrapped function, and this ended up making it unnecessary. The flush is now called at the end of the block. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>

danieltahara approved these changes Sep 10, 2018

View reviewed changes

LK4D4 reviewed Sep 10, 2018

View reviewed changes

sougou merged commit fc41fc3 into vitessio:master Sep 11, 2018

LK4D4 mentioned this pull request Sep 11, 2018

[go/mysql] use sync.Pool for reusing bufio writers #4188

Closed

sougou deleted the mysql branch September 11, 2018 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mysql: improve code structure for buffered vs unbuffered writes #4190

mysql: improve code structure for buffered vs unbuffered writes #4190

sougou commented Sep 9, 2018 •

edited

Loading

This comment was marked as resolved.

danieltahara Sep 9, 2018

danieltahara Sep 9, 2018

sougou Sep 9, 2018

danieltahara Sep 9, 2018

sougou Sep 9, 2018

danieltahara commented Sep 9, 2018 via email

sougou commented Sep 9, 2018

danieltahara Sep 9, 2018

sougou Sep 10, 2018

danieltahara Sep 10, 2018

sougou Sep 10, 2018

danieltahara Sep 10, 2018

danieltahara left a comment

danieltahara Sep 10, 2018

LK4D4 Sep 10, 2018

sougou Sep 10, 2018

LK4D4 commented Sep 10, 2018

sougou commented Sep 11, 2018

mysql: improve code structure for buffered vs unbuffered writes #4190

mysql: improve code structure for buffered vs unbuffered writes #4190

Conversation

sougou commented Sep 9, 2018 • edited Loading

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltahara commented Sep 9, 2018 via email

sougou commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltahara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LK4D4 commented Sep 10, 2018

sougou commented Sep 11, 2018

sougou commented Sep 9, 2018 •

edited

Loading