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

*: support variable log_bin #9343

Merged
merged 21 commits into from
Feb 20, 2019
Merged

Conversation

aliiohs
Copy link
Contributor

@aliiohs aliiohs commented Feb 18, 2019

issue:#9201

Support log_bin

i changed some files,like:
sessionctx/variable/sysvar.go:395
sessionctx/variable/sysvar.go:751
sessionctx/variable/varsutil.go:351
executor/set_test.go:223

@codecov-io
Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #9343 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9343      +/-   ##
==========================================
- Coverage   67.14%   67.13%   -0.02%     
==========================================
  Files         373      373              
  Lines       78136    78136              
==========================================
- Hits        52466    52457       -9     
- Misses      20973    20980       +7     
- Partials     4697     4699       +2
Impacted Files Coverage Δ
sessionctx/variable/sysvar.go 100% <100%> (ø) ⬆️
sessionctx/variable/varsutil.go 25.92% <50%> (ø) ⬆️
ddl/session_pool.go 82.75% <0%> (-10.35%) ⬇️
store/tikv/scan.go 73.94% <0%> (-3.37%) ⬇️
ddl/delete_range.go 73.54% <0%> (-1.59%) ⬇️
expression/schema.go 93.75% <0%> (-0.79%) ⬇️
infoschema/infoschema.go 77.63% <0%> (+1.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e499da9...1cde77b. Read the comment docs.

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.

Thanks for your contribution, @aliiohs !
Please add a test case, e.g. select @@log_bin;
Rest LGTM.

@WangXiangUSTC WangXiangUSTC added contribution This PR is from a community contributor. component/binlog labels Feb 18, 2019
tidb-server/main.go Outdated Show resolved Hide resolved
tidb-server/main.go Outdated Show resolved Hide resolved
tidb-server/main.go Outdated Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 18, 2019
@jackysp jackysp changed the title Support variable log_bin *: support variable log_bin Feb 18, 2019
@aliiohs
Copy link
Contributor Author

aliiohs commented Feb 19, 2019

@jackysp i already added the test case for 'log_bin'

@@ -340,6 +340,15 @@ func ValidateSetSystemVar(vars *SessionVars, name string, value string) (string,
return "0", nil
}
return value, ErrWrongValueForVar.GenWithStackByArgs(name, value)
case LogBin:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put it to line 335? Then we can remove line344-351.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LogBin need to return "ON" or "OFF", not "0" or "1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zimulala so i will merge 'LogBin' and 'TiDBEnableTablePartition'

Copy link
Contributor

Choose a reason for hiding this comment

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

No. I mean you can merge LogBin with GeneralLog. TiDBEnableTablePartition has the value of AUTO, which is different from LogBin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return value is "0" or “1”, is it expected? @zimulala

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's OK. We can handle LogBin like SQLLogBin. Then we can remove BoolToStatusStr.
We will handle #9357 in the next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i will change the code later. @zimulala

@@ -348,7 +348,7 @@ func ValidateSetSystemVar(vars *SessionVars, name string, value string) (string,
return value, nil
}
return value, ErrWrongValueForVar.GenWithStackByArgs(name, value)
case TiDBEnableTablePartition:
case TiDBEnableTablePartition, LogBin:
Copy link
Contributor

Choose a reason for hiding this comment

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

If the log_bin == "2", the return value is "auto", is it expected?

Copy link
Contributor Author

@aliiohs aliiohs Feb 19, 2019

Choose a reason for hiding this comment

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

@winkyao @zimulala I think I changed the return value to "0" and "1" is not a good way, so i want to keep it in independent case.

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM, @jackysp PTAL

@@ -15,6 +15,7 @@ package executor_test

import (
"context"
"github.com/pingcap/tidb/config"
Copy link
Member

Choose a reason for hiding this comment

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

Keep the sys libs seperate with the 3rd libs. Please move it down to line 20.

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 already finished it

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

@jackysp
Copy link
Member

jackysp commented Feb 20, 2019

/run-all-tests

@WangXiangUSTC WangXiangUSTC added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 20, 2019
@WangXiangUSTC
Copy link
Contributor

/run-all-tests

@ngaut ngaut merged commit 8f3c74c into pingcap:master Feb 20, 2019
@aliiohs aliiohs deleted the support_log_bin_2019_02_18_1 branch March 3, 2019 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/binlog contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants