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

truncate info field for show processlist and show full processlist support #4739

Merged
merged 16 commits into from Oct 26, 2017

Conversation

Projects
None yet
8 participants
@Darren
Contributor

Darren commented Oct 10, 2017

this PR fixes #4737

@sre-bot

This comment has been minimized.

Show comment
Hide comment
@sre-bot

sre-bot Oct 10, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

sre-bot commented Oct 10, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Oct 10, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Oct 10, 2017

CLA assistant check
All committers have signed the CLA.

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Oct 10, 2017

Member

@Darren
It looks like your git email is not set to your github email, so the CLA cannot be signed.

Member

coocood commented Oct 10, 2017

@Darren
It looks like your git email is not set to your github email, so the CLA cannot be signed.

Show outdated Hide outdated executor/show.go
@@ -175,6 +175,14 @@ func (e *ShowExec) fetchShowProcessList() error {
if len(pi.Info) != 0 {
t = uint64(time.Since(pi.Time) / time.Second)
}
var info string
if e.Full {

This comment has been minimized.

@shenli

shenli Oct 10, 2017

Member

Could you add a test case?

@shenli

shenli Oct 10, 2017

Member

Could you add a test case?

This comment has been minimized.

@Darren

Darren Oct 10, 2017

Contributor

I tried adding test but failed.

Since testSuite in tidb/executor has no SessionManager, full tests on show processlist seems not possible here.

Can you give me some advice?

@Darren

Darren Oct 10, 2017

Contributor

I tried adding test but failed.

Since testSuite in tidb/executor has no SessionManager, full tests on show processlist seems not possible here.

Can you give me some advice?

This comment has been minimized.

@shenli

shenli Oct 11, 2017

Member

You can create a util/mock.Context and set a mocked SessionManager to it.

@shenli

shenli Oct 11, 2017

Member

You can create a util/mock.Context and set a mocked SessionManager to it.

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Oct 13, 2017

Member

@Darren After PR #4763 it will be easier to write unit test case for ShowProcessList.

Member

shenli commented Oct 13, 2017

@Darren After PR #4763 it will be easier to write unit test case for ShowProcessList.

if e.Full {
info = pi.Info
} else {
info = fmt.Sprintf("%.100v", pi.Info)

This comment has been minimized.

@tiancaiamao

tiancaiamao Oct 16, 2017

Contributor

You mean truncate info field length rather than item counts?

@tiancaiamao

tiancaiamao Oct 16, 2017

Contributor

You mean truncate info field length rather than item counts?

This comment has been minimized.

@Darren

Darren Oct 16, 2017

Contributor

yes, truncate the info field, the value 100 is from mysql source code:

sql/sql_const.h:#define PROCESS_LIST_WIDTH 100

mysqld_list_processes is defined in sql/sql_show.cc

mysql truncates the info fields byte-wize, but using go's fmt.Sprint above it truncates at the unicode width boudary, but i don't think that would be an issue, do you?

@Darren

Darren Oct 16, 2017

Contributor

yes, truncate the info field, the value 100 is from mysql source code:

sql/sql_const.h:#define PROCESS_LIST_WIDTH 100

mysqld_list_processes is defined in sql/sql_show.cc

mysql truncates the info fields byte-wize, but using go's fmt.Sprint above it truncates at the unicode width boudary, but i don't think that would be an issue, do you?

This comment has been minimized.

@tiancaiamao

tiancaiamao Oct 16, 2017

Contributor

ok

@tiancaiamao
@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Oct 20, 2017

Member

@Darren Any update?

Member

shenli commented Oct 20, 2017

@Darren Any update?

@zz-jason zz-jason added this to the 1.1 milestone Oct 26, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Oct 26, 2017

@Darren

This comment has been minimized.

Show comment
Hide comment
@Darren

Darren Oct 26, 2017

Contributor

@shenli

sorry for the delay, I've added test, please take a look.

Contributor

Darren commented Oct 26, 2017

@shenli

sorry for the delay, I've added test, please take a look.

@@ -4524,10 +4524,11 @@ ShowStmt:
User: $4.(*auth.UserIdentity),
}
}
| "SHOW" "PROCESSLIST"

This comment has been minimized.

@zimulala

zimulala Oct 26, 2017

Member

Please add tests to parser_test.go.

@zimulala

zimulala Oct 26, 2017

Member

Please add tests to parser_test.go.

This comment has been minimized.

@Darren

Darren Oct 26, 2017

Contributor

done

@Darren

Darren Oct 26, 2017

Contributor

done

Show outdated Hide outdated executor/show_test.go
Show outdated Hide outdated executor/show_test.go
Show outdated Hide outdated executor/show_test.go
@Darren

This comment has been minimized.

Show comment
Hide comment
@Darren

Darren Oct 26, 2017

Contributor

@zimulala

Is tidb/hooks/pre-commit used by all contributors? I first used this, but there are some typos in the current code base, and I can not commit unless I fix that. So I delete pre-commit in my git hooks.

Contributor

Darren commented Oct 26, 2017

@zimulala

Is tidb/hooks/pre-commit used by all contributors? I first used this, but there are some typos in the current code base, and I can not commit unless I fix that. So I delete pre-commit in my git hooks.

Darren added some commits Oct 26, 2017

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala
Member

zimulala commented Oct 26, 2017

PTAL @zhexuany

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala

zimulala Oct 26, 2017

Member

LGTM

Member

zimulala commented Oct 26, 2017

LGTM

@zimulala zimulala added status/LGT2 and removed status/LGT1 labels Oct 26, 2017

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala

zimulala Oct 26, 2017

Member

/run-all-tests

Member

zimulala commented Oct 26, 2017

/run-all-tests

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 26, 2017

Member

/run-unit-test

Member

zz-jason commented Oct 26, 2017

/run-unit-test

@zz-jason zz-jason merged commit e40ad67 into pingcap:master Oct 26, 2017

12 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 72.414%
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@Darren Darren deleted the Darren:show-full-processlist branch Oct 27, 2017

dbjoa added a commit to cloud-pi/tidb that referenced this pull request Oct 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment