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

tikvclient: adjust log level in store/tikv/client_batch.go #12302

Merged
merged 9 commits into from Sep 25, 2019

Conversation

hicqu
Copy link
Contributor

@hicqu hicqu commented Sep 23, 2019

What problem does this PR solve?

Some error logs could make users confused.

What is changed and how it works?

Change some log level from Error to Warn, because TiDB can auto recover from those errors.

Check List

Tests
No code. Because it just adjusts log level.

Code changes
Checked.

Side effects
No.

Related changes
Need to cherry-pick to release-3.0.

Release note
No.

@@ -215,8 +215,8 @@ func (c *batchCommandsClient) send(request *tikvpb.BatchCommandsRequest, entries
c.batched.Store(requestID, entries[i])
}
if err := c.client.Send(request); err != nil {
logutil.BgLogger().Error(
"batch commands send error",
logutil.BgLogger().Warn(
Copy link
Member

Choose a reason for hiding this comment

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

can we set it to debug or info? or can we not print this message since the user no need to worry about these kinds of logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to set it to info if you think it's better.

Except waitting responses. In this case logs will be printed when
responses are not received after 60s.

Signed-off-by: qupeng <qupeng@pingcap.com>
@codecov
Copy link

codecov bot commented Sep 23, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12302   +/-   ##
===========================================
  Coverage   80.8442%   80.8442%           
===========================================
  Files           454        454           
  Lines         99756      99756           
===========================================
  Hits          80647      80647           
  Misses        13318      13318           
  Partials       5791       5791

@hicqu
Copy link
Contributor Author

hicqu commented Sep 23, 2019

PTAL @zz-jason @lysu

@hicqu
Copy link
Contributor Author

hicqu commented Sep 25, 2019

PTAL @zz-jason @tiancaiamao

@zz-jason
Copy link
Member

zz-jason commented Sep 25, 2019

@hicqu please follow the Commit Message and Pull Request Style guide to reformat the PR title.

BTW, please add some proper labels for this PR.

@hicqu hicqu changed the title adjust log level in store/tikv/client_batch.go tikvclient: adjust log level in store/tikv/client_batch.go Sep 25, 2019
@hicqu
Copy link
Contributor Author

hicqu commented Sep 25, 2019

PTAL @zz-jason

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added component/DDL-need-LGT3 status/LGT1 Indicates that a PR has LGTM 1. labels Sep 25, 2019
@lysu lysu added require-LGT3 Indicates that the PR requires three LGTM. and removed component/DDL-need-LGT3 labels Sep 25, 2019
@@ -215,8 +215,8 @@ func (c *batchCommandsClient) send(request *tikvpb.BatchCommandsRequest, entries
c.batched.Store(requestID, entries[i])
}
if err := c.client.Send(request); err != nil {
logutil.BgLogger().Error(
"batch commands send error",
logutil.BgLogger().Info(
Copy link
Collaborator

Choose a reason for hiding this comment

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

which metric can we use to identity send failure if user use error/warn level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems in the module there is no such metric. Is it better to put the metric into SendRequest callers?

@lysu
Copy link
Collaborator

lysu commented Sep 25, 2019

LGTM

@lysu lysu removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 25, 2019
@lysu lysu added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 25, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@hicqu hicqu merged commit c15fdd2 into pingcap:master Sep 25, 2019
@hicqu hicqu deleted the recycle-conn-without-log branch September 25, 2019 07:13
@sre-bot
Copy link
Contributor

sre-bot commented Sep 25, 2019

cherry pick to release-3.0 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv require-LGT3 Indicates that the PR requires three LGTM. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants