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

*: fix update relay processing order #142

Merged
merged 9 commits into from
May 20, 2019

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented May 10, 2019

What problem does this PR solve?

in DM-worker UpdateRelayConfig function, the check of update source ID is not allowed is before new config loaded, which will always fail.
Besides I find we will get the following error after we update a relay config. This is caused by we forget to close binlog replication syncer after relay paused

2019/05/10 06:19:52.383 relay.go:176: [error] [relay] process exit with error ERROR 1236 (HY000): A slave with the same server_uuid/server_id as this slave has connected to the master; the first event 'bin.000007' at 27622680, the last event read from './bin.000007' at 27622993, the last byte read from './bin.000007' at 27622993.
github.com/pingcap/errors.AddStack
        /root/.gvm/pkgsets/go1.12/global/pkg/mod/github.com/pingcap/errors@v0.11.0/errors.go:174
github.com/pingcap/errors.Trace
        /root/.gvm/pkgsets/go1.12/global/pkg/mod/github.com/pingcap/errors@v0.11.0/juju_adaptor.go:12
github.com/pingcap/dm/relay.(*Relay).process
        /root/code/gopath/src/github.com/pingcap/dm/relay/relay.go:357
github.com/pingcap/dm/relay.(*Relay).Process
        /root/code/gopath/src/github.com/pingcap/dm/relay/relay.go:173
github.com/pingcap/dm/dm/worker.(*RelayHolder).run
        /root/code/gopath/src/github.com/pingcap/dm/dm/worker/relay.go:107
github.com/pingcap/dm/dm/worker.(*RelayHolder).resumeRelay.func1
        /root/code/gopath/src/github.com/pingcap/dm/dm/worker/relay.go:200
runtime.goexit
        /root/.gvm/gos/go1.12/src/runtime/asm_amd64.s:1337
2019/05/10 06:19:52.383 relay.go:113: [error] process error with type UnknownError:

What is changed and how it works?

  1. Load the new DM-worker config before any validation
  2. close binlog syncer in TCPReader.Close

Check List

Tests

  • Integration test
  • Manual test (add detailed scripts or steps below)
    • deploy a DM cluster
    • run update-relay -w worker_host:worker_port path_to_new_dm_worker_config in dmctl
    • check update-relay is success

new DM-worker config must be loaded before any validation
@amyangfei amyangfei added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix labels May 10, 2019
@amyangfei amyangfei added status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels May 10, 2019
@amyangfei amyangfei added 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 May 10, 2019
@amyangfei
Copy link
Contributor Author

/run-all-tests

4 similar comments
@amyangfei
Copy link
Contributor Author

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@amyangfei
Copy link
Contributor Author

PTAL @GregoryIan @csuzhangxc

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc 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 May 13, 2019
@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #142 into master will increase coverage by 0.0766%.
The diff coverage is 12.5%.

@@               Coverage Diff                @@
##             master       #142        +/-   ##
================================================
+ Coverage   29.8727%   29.9493%   +0.0766%     
================================================
  Files           115        115                
  Lines         12647      12638         -9     
================================================
+ Hits           3778       3785         +7     
+ Misses         8436       8420        -16     
  Partials        433        433

@amyangfei
Copy link
Contributor Author

codecov diff hit means the coverage rate of diff code, it is not stable now, may be we can ignore

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

@csuzhangxc csuzhangxc added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels May 20, 2019
@amyangfei
Copy link
Contributor Author

/run-all-tests

2 similar comments
@amyangfei
Copy link
Contributor Author

/run-all-tests

@zhouqiang-cl
Copy link
Contributor

/run-all-tests

@amyangfei amyangfei merged commit 43b063d into pingcap:master May 20, 2019
@amyangfei amyangfei deleted the fix-update-relay-cfg branch May 20, 2019 07:20
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants