properly suppress begin...commit in autocommit logs #4827
Merged
Conversation
This file contains 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
The previous (untested) implementation turned out to be in the wrong place in the TabletServer execution tier and did not properly log the actual statements being executed. Implement this the right way by returning the statements that were really executed out from the TxPool, then using those to determine whether or not to log the statement. Signed-off-by: Michael Demmer <[email protected]>
rafael
approved these changes
Apr 19, 2019
Signed-off-by: Michael Demmer <[email protected]>
|
@rafael pointed out that the query executor stats shown in the debug UI also had misleading BEGIN / COMMIT operations so I pushed another commit that removes those as well. |
demmer
commented
Apr 19, 2019
| @@ -773,13 +776,25 @@ func (tsv *TabletServer) Begin(ctx context.Context, target *querypb.Target, opti | |||
| "Begin", "begin", nil, | |||
| target, options, true /* isBegin */, false, /* allowOnShutdown */ | |||
| func(ctx context.Context, logStats *tabletenv.LogStats) error { | |||
| defer tabletenv.QueryStats.Record("BEGIN", time.Now()) | |||
| startTime := time.Now() | |||
There was a problem hiding this comment.
This is a slight behavior change -- previously the stats were incremented when the txThrottler rejects the operation, but now it's only counted if the begin actually occurs.
I think that's an improvement personally.
sougou
reviewed
Apr 22, 2019
|
Maybe. I considered that but it seemed like an awkward layering violation in the design. Returning out the statements that were executed seemed like the best compromise for getting that info. |
setassociative
pushed a commit
to tinyspeck/vitess
that referenced
this pull request
Apr 26, 2019
It's been busy and exciting in vitess upstream. Some changes that felt worth calling out as they could impact assumptions about behavior: - vitessio#4832 — this adds a timeout that is 3x the `healthCheckInterval` which at which point the tablet will stop reporting as healthy - vitessio#4814 — this changes when command line args are used by vttablet - vitessio#4811 — a planned reparent now sets `super_read_only` - vitessio#4805 — behavior coming back from a failed backup is different **Slack changes** @demmer vitessio#4827 Correct suppress logging for begin...commit in autocommit @rafael vitessio#4824 adds timeouts for all statements @demmer vitessio#4826 adds logging of stack traces as opt-in @demmer vitessio#4819 remove begin/commit logs from autocommit txns @demmer vitessio#4796 improves support for vtexplain for begin/dml/dml/commit txns **Non slack changes** vitessio#4839 Improve behavior with reference table routing & vreplication vitessio#4833 Support query routing given there could be multiple targets for a table vitessio#4832 tablets get new health check behavior (health checks time out) vitessio#4837 refresh bug around row streamer handling creds vitessio#4830 apply a default value to the db_name attribute in local_metadata vitessio#4785 SHOW SCHEMAS aliased to SHOW DATABASES vitessio#4829 add experimental support for split clone & vertical split clone via vreplication vitessio#4822 do not normalize in order by vitessio#4791 Adds vreplication row streaming support vitessio#4814 command line flags now used regardless of management mode vitessio#4811 vttablet sets super_read_only during planned reparent vitessio#4803 xtrabackup testing vitessio#4727 support multiple vttablets running against one mysql instance vitessio#4746 TopoCat can now produce JSON output vitessio#4805 mysqld is now restarted after a failed backup vitessio#4685 macos bootstrap bug vitessio#4874 ZK opts and java land vitessio#4695 Adds support for xtrabackup vitessio#4794 fixes build failures vitessio#4725 changes mysql8.0 start args vitessio#4736 introduce new states to support vreplication vitessio#4788 expands orc error **Docs etc** vitessio#4831 lint vitessio#4827 docs vitessio#4816 const declaration cleanup vitessio#4820 const declaration cleanup vitessio#4825 docs vitessio#4818 docs vitessio#4809 docs vitessio#4812 moves consts around vitessio#4813 docs vitessio#4808 docs vitessio#4800 docs vitessio#4795 docs
setassociative
pushed a commit
to tinyspeck/vitess
that referenced
this pull request
Jun 5, 2019
Original Message: The previous (untested) implementation turned out to be in the wrong place in the TabletServer execution tier and did not properly log the actual statements being executed. Implement this the right way by returning the statements that were really executed out from the TxPool, then using those to determine whether or not to log the statement. Original PR: vitessio#4827
setassociative
pushed a commit
to tinyspeck/vitess
that referenced
this pull request
Jun 5, 2019
Original Message: The previous (untested) implementation turned out to be in the wrong place in the TabletServer execution tier and did not properly log the actual statements being executed. Implement this the right way by returning the statements that were really executed out from the TxPool, then using those to determine whether or not to log the statement. Original PR: vitessio#4827
setassociative
pushed a commit
to tinyspeck/vitess
that referenced
this pull request
Jun 5, 2019
* Adds timeouts for all kinds of statements. Power to the clients! Signed-off-by: Rafael Chacon <[email protected]> * Cherry-pick: suppress begin...commit in autocommit logs Original Message: The previous (untested) implementation turned out to be in the wrong place in the TabletServer execution tier and did not properly log the actual statements being executed. Implement this the right way by returning the statements that were really executed out from the TxPool, then using those to determine whether or not to log the statement. Original PR: vitessio#4827 * Cherry pick: add optional tag-based filtering for query logging Original PR: vitessio#4895 * bootstrap.sh: Remove unused unused command. The deprecated `unused` command has been removed from the source repository, so this now fails. We no longer use `unused` so we can just drop the line that tries to install it. Signed-off-by: Anthony Yeh <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Actually implement the proper logging of which statements are executed by Begin... Commit.
Details
The previous (untested) implementation in #4819 turned out to be in the wrong place in the TabletServer execution tier and did not properly suppress the logs in ExecuteBatch autocommit.
Implement this the right way by returning the statements that were really executed out from the TxPool, then using those to determine whether or not to log the statement.
This has the nice side-benefit of including in the transaction logging the statements executed to create a transaction with a given isolation level.
Testing
Verified (this time) that when using autocommit transactions with passthrough DML, the query logger just logs the DML statements and not misleading Begin/Commit.