vttablet: benchmarks and two small optimizations#7560
Merged
systay merged 3 commits intovitessio:masterfrom Mar 1, 2021
Merged
vttablet: benchmarks and two small optimizations#7560systay merged 3 commits intovitessio:masterfrom
systay merged 3 commits intovitessio:masterfrom
Conversation
Signed-off-by: Vicent Marti <vmg@strn.cat>
Calling `cp.MysqlParams` once per event is really wasteful because it must prepare a Parameters struct including all the authentication credentials for the connection. We only need the active database name to process the event, so ask just for that. Signed-off-by: Vicent Marti <vmg@strn.cat>
There is a bad merge in this code that is causing `ReloadAt` to be called two times in a row. Previously, the schema was _always_ being reloaded when parsing a DDL statement. @sougou changed this so the schema only reloads when the statement must be sent upstream, but then Rohit brought back the functionality of _always_ reloading the DDL without removing the case where we were already reloading for sent statements. Signed-off-by: Vicent Marti <vmg@strn.cat>
vmg
requested review from
harshit-gangal,
shlomi-noach,
sougou and
systay
as code owners
vmg
commented
Feb 26, 2021
| // Reload schema only if the DDL change is relevant. | ||
| // TODO(sougou): move this back to always load after | ||
| // the schema reload bug is fixed. | ||
| vs.se.ReloadAt(context.Background(), vs.pos) |
Collaborator
Author
Member
There was a problem hiding this comment.
@vmg, yes this can be removed. I thought I had already removed the duplication, thanks for catching this.
Contributor
There was a problem hiding this comment.
Nice catch! The call should be removed.
vmg
commented
Feb 26, 2021
| @@ -481,7 +472,7 @@ func (vs *vstreamer) parseEvent(ev mysql.BinlogEvent) ([]*binlogdatapb.VEvent, e | |||
| } | |||
| vs.se.ReloadAt(context.Background(), vs.pos) | |||
Collaborator
Author
There was a problem hiding this comment.
Note that the call is duplicated here.
shlomi-noach
approved these changes
Mar 1, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is a small benchmark suite for
vttabletqueries which I've started running to find hotspots.So far, I found two fixes that are trivial and give a measurable improvement in the flame graphs.
vstreameris hot!Related Issue(s)
Checklist
Deployment Notes
Impacted Areas in Vitess
Components that this PR will affect: