Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

*: combine etcd log into dm-master #360

Merged
merged 20 commits into from
Nov 19, 2019
Merged

*: combine etcd log into dm-master #360

merged 20 commits into from
Nov 19, 2019

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Nov 15, 2019 •
edited
Loading

What problem does this PR solve?

combine etcd log into dm-master

What is changed and how it works?

  • update vendor to support etcd v3.4.3 (for zap log in etcd)
  • update golang version to 1.13 (to compatible with TiDB, PD and etcd)
  • combine etcd log into dm-master
go get -u github.com/pingcap/tidb@b274eb2
go get -u github.com/pingcap/tidb-tools@48d5e90
go get -u github.com/pingcap/parser@3b43b46
go get -u github.com/pingcap/pd@2488cb9
go get -u go.etcd.io/etcd@3cf2f69
go mod tidy

[2019/11/18 14:42:57.180 +08:00] [INFO] [printer.go:54] ["Welcome to dm-master"] ["Release Version"=] ["Git Commit Hash"=64711dd8d39518414123f13a46299b478e4ff72d] ["Git Branch"=etcd-log] ["UTC Build Time"="2019-11-18 06:42:26"] ["Go Version"="go version go1.13.4 darwin/amd64"]
[2019/11/18 14:42:57.180 +08:00] [INFO] [main.go:58] ["dm-master config"="{\"log-level\":\"info\",\"log-file\":\"\",\"log-rotate\":\"day\",\"rpc-timeout\":\"30s\",\"rpc-rate-limit\":10,\"rpc-rate-burst\":40,\"master-addr\":\"127.0.0.1:8261\",\"deploy\":{},\"config-file\":\"dm-master.toml\",\"name\":\"dm-master-XuechengdeMacBook-Pro.local\",\"data-dir\":\"default.dm-master-XuechengdeMacBook-Pro.local\",\"peer-urls\":\"http://127.0.0.1:8291\",\"advertise-peer-urls\":\"http://127.0.0.1:8291\",\"initial-cluster\":\"dm-master-XuechengdeMacBook-Pro.local=http://127.0.0.1:8291\",\"initial-cluster-state\":\"new\",\"join\":\"\"}"]
[2019/11/18 14:42:57.180 +08:00] [INFO] [server.go:103] ["config after join prepared"] [config="{\"log-level\":\"info\",\"log-file\":\"\",\"log-rotate\":\"day\",\"rpc-timeout\":\"30s\",\"rpc-rate-limit\":10,\"rpc-rate-burst\":40,\"master-addr\":\"127.0.0.1:8261\",\"deploy\":{},\"config-file\":\"dm-master.toml\",\"name\":\"dm-master-XuechengdeMacBook-Pro.local\",\"data-dir\":\"default.dm-master-XuechengdeMacBook-Pro.local\",\"peer-urls\":\"http://127.0.0.1:8291\",\"advertise-peer-urls\":\"http://127.0.0.1:8291\",\"initial-cluster\":\"dm-master-XuechengdeMacBook-Pro.local=http://127.0.0.1:8291\",\"initial-cluster-state\":\"new\",\"join\":\"\"}"]
[2019/11/18 14:42:57.180 +08:00] [INFO] [etcd.go:117] ["configuring peer listeners"] [component="embed etcd"] [listen-peer-urls="[http://127.0.0.1:8291]"]
[2019/11/18 14:42:57.181 +08:00] [INFO] [etcd.go:127] ["configuring client listeners"] [component="embed etcd"] [listen-client-urls="[http://127.0.0.1:8261]"]

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer status/WIP This PR is still work in progress type/feature New feature labels Nov 15, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

1 similar comment
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc csuzhangxc changed the title *: combine etcd log into dm-master.log *: combine etcd log into dm-master Nov 15, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

2 similar comments
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

1 similar comment
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

1 similar comment
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

4 similar comments
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

csuzhangxc
csuzhangxc commented Nov 16, 2019
View reviewed changes
pkg/log/log.go
@@ -89,7 +90,6 @@ func InitLogger(cfg *Config) error {
Level: cfg.Level,
File: pclog.FileLogConfig{
Filename: cfg.File,
LogRotate: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

LogRotate removed in current pingcap/log.

@csuzhangxc csuzhangxc added needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Nov 16, 2019
@csuzhangxc
Copy link
Member Author

@GregoryIan @WangXiangUSTC PTAL

@csuzhangxc csuzhangxc mentioned this pull request Nov 16, 2019
Merged
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

WangXiangUSTC
WangXiangUSTC reviewed Nov 18, 2019
View reviewed changes
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

rest LGTM

dm/master/config.go Outdated Show resolved Hide resolved
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Nov 18, 2019
IANTHEREAL
IANTHEREAL reviewed Nov 19, 2019
View reviewed changes
Copy link
Collaborator

@IANTHEREAL IANTHEREAL left a comment

Choose a reason for hiding this comment

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

Rest LGTM

tests/http_apis/run.sh Outdated
@@ -31,6 +31,7 @@ function run() {
task_data=`cat $WORK_DIR/task.yaml.bak`
rm $WORK_DIR/task.yaml.bak
echo $task_data
sleep 2 # wait for embed HTTP service avaible
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add a simple backoff function to wait it ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

added backoff in aa3d4e7.

syncer/db_test.go Outdated
@@ -275,7 +275,10 @@ func (s *testDBSuite) TestTimezone(c *C) {
continue
}

rowid := ev.Rows[0][0].(int32)
rowid, ok := ev.Rows[0][0].(int32)
Copy link
Collaborator

Choose a reason for hiding this comment

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

any concurrent case to run? or does not resetBinlogSyncer help to filter other cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

I revert this change (in aa3d4e7) and will split the unit test cases (with MySQL) into different containers in CI later.

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

IANTHEREAL
IANTHEREAL reviewed Nov 19, 2019
View reviewed changes
Copy link
Collaborator

@IANTHEREAL IANTHEREAL left a comment

Choose a reason for hiding this comment

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

LGTM

@IANTHEREAL IANTHEREAL added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Nov 19, 2019
@csuzhangxc csuzhangxc merged commit a64b26d into pingcap:master Nov 19, 2019
@csuzhangxc csuzhangxc deleted the etcd-log branch November 19, 2019 09:49
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@csuzhangxc
*: combine etcd log into dm-master (pingcap#360)
1ab4ee2
@csuzhangxc csuzhangxc removed the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@WangXiangUSTC WangXiangUSTC WangXiangUSTC left review comments

@IANTHEREAL IANTHEREAL IANTHEREAL approved these changes

Assignees
No one assigned
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@csuzhangxc @WangXiangUSTC @IANTHEREAL