Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

[Draft] Control vtgate buffer polling with mysql_conn_buffer_polling parameter #254

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

roderickyao
Copy link

Description

Adding an option for the vtgate to pool the connection buffer instead of creating new buffer for each new connection.

Related Issue(s)

Slack vtgate doses

@roderickyao roderickyao requested review from henryr and vmogilev July 21, 2022 20:38
Copy link

@brirams brirams left a comment

Choose a reason for hiding this comment

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

looks good sans a few nits! going to take this branch and default the pooling to on so we can do some testing!

@@ -183,31 +186,32 @@ type Listener struct {
PreHandleFunc func(context.Context, net.Conn, uint32) (net.Conn, error)
}

// NewFromListener creates a new mysql listener from an existing net.Listener
func NewFromListener(l net.Listener, authServer AuthServer, handler Handler, connReadTimeout time.Duration, connWriteTimeout time.Duration) (*Listener, error) {
// NewFromListener creares a new mysql listener from an existing net.Listener
Copy link

Choose a reason for hiding this comment

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

would revert this bit

Comment on lines +1 to +2
//go:build gofuzz
// +build gofuzz
Copy link

Choose a reason for hiding this comment

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

unclear what this change is about. should we revert it?

@@ -13,7 +16,6 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
// +build gofuzz
Copy link

Choose a reason for hiding this comment

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

same comment as above.

Comment on lines +264 to +265
c.bufferedReader.Reset(nil)
readersPool.Put(c.bufferedReader)
Copy link

Choose a reason for hiding this comment

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

would be worth checking that the bufferedReader isn't nil before doing this.

@@ -72,6 +72,11 @@ var (
mysqlConnWriteTimeout = flag.Duration("mysql_server_write_timeout", 0, "connection write timeout")
mysqlQueryTimeout = flag.Duration("mysql_server_query_timeout", 0, "mysql query timeout")

// TODO: enable this once the loadtest is done
// mysqlConnBufferPooling = flag.Bool("mysql_conn_buffer_polling", false, "If set, vtgate will pool connection buffers")
Copy link

Choose a reason for hiding this comment

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

nit for later: would rename flag to have consistent prefix with other flags in this file. something like mysql_server_read_conn_pooling? would also update flag description to specify that it's connection reading as connection writing is already pooled.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants