Skip to content
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

log: move autocommit varable value into connection info #12310

Merged
merged 8 commits into from
Oct 9, 2019

Conversation

imtbkcat
Copy link

@imtbkcat imtbkcat commented Sep 23, 2019

What problem does this PR solve?

Some applications will set autocommit variable every time when they execute queries. This action will make tidb log full of set session var.

What is changed and how it works?

I remove this log info for autocommit variable and add autocommit value when print connection information into log.
just like: ["command dispatched failed"] [conn=2] [connInfo="id:2, addr:127.0.0.1:52703 status:0, collation:utf8_general_ci, user:root autocommit:0"]

Check List

Tests

  • No code

Code changes

  • no

Side effects

  • no

Related changes

  • Need to cherry-pick to the release branch

Release note

@imtbkcat imtbkcat added type/enhancement The issue or PR belongs to an enhancement. type/usability component/util labels Sep 23, 2019
@imtbkcat imtbkcat requested a review from jackysp September 23, 2019 08:09
@codecov
Copy link

codecov bot commented Sep 23, 2019

Codecov Report

Merging #12310 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12310   +/-   ##
===========================================
  Coverage   79.9653%   79.9653%           
===========================================
  Files           460        460           
  Lines        103221     103221           
===========================================
  Hits          82541      82541           
  Misses        14684      14684           
  Partials       5996       5996

server/conn.go Outdated
autoCommit = 1
}
return fmt.Sprintf("id:%d, addr:%s status:%d, collation:%s, user:%s autocommit:%d",
cc.connectionID, cc.bufReadConn.RemoteAddr(), cc.ctx.Status(), collationStr, cc.user, autoCommit,
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to format the output of cc.ctx.Status(). Because it already contains the autocommit status.

@@ -188,7 +188,9 @@ func (e *SetExecutor) setSysVariable(name string, v *expression.VarAssignment) e
valStr, err = value.ToString()
terror.Log(err)
}
logutil.BgLogger().Info("set session var", zap.Uint64("conn", sessionVars.ConnectionID), zap.String("name", name), zap.String("val", valStr))
if name != variable.AutoCommit {
logutil.BgLogger().Info("set session var", zap.Uint64("conn", sessionVars.ConnectionID), zap.String("name", name), zap.String("val", valStr))
Copy link
Member

Choose a reason for hiding this comment

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

I think we could simply change it to the debug level.
\cc @shenli @coocood

@coocood
Copy link
Member

coocood commented Sep 23, 2019

@jackysp @imtbkcat
We can log it only when the new variable value is different, it doesn't lose any variable information.

@imtbkcat imtbkcat force-pushed the var_log branch 2 times, most recently from 93bdb0b to f6f925c Compare September 24, 2019 06:41
@coocood
Copy link
Member

coocood commented Sep 25, 2019

LGTM

@imtbkcat imtbkcat added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 29, 2019
@imtbkcat
Copy link
Author

imtbkcat commented Oct 9, 2019

/run-all-tests tidb-test=pr/875

@imtbkcat
Copy link
Author

imtbkcat commented Oct 9, 2019

/run-all-tests tidb-test=pr/875

@imtbkcat
Copy link
Author

imtbkcat commented Oct 9, 2019

/run-common-test tidb-test=pr/875

@imtbkcat
Copy link
Author

imtbkcat commented Oct 9, 2019

/run-all-tests tidb-test=pr/875

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@imtbkcat imtbkcat added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. needs-cherry-pick-3.0 and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

@imtbkcat merge failed.

@jackysp
Copy link
Member

jackysp commented Oct 9, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

@imtbkcat merge failed.

@zyxbest
Copy link
Contributor

zyxbest commented Oct 9, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

@imtbkcat merge failed.

@imtbkcat imtbkcat removed the status/can-merge Indicates a PR has been approved by a committer. label Oct 9, 2019
@zyxbest
Copy link
Contributor

zyxbest commented Oct 9, 2019

/run-check_dev

@imtbkcat imtbkcat merged commit 4642b54 into pingcap:master Oct 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Oct 9, 2019

cherry pick to release-3.1 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/util status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement. type/usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants