relay: don't purge relay logs when connected to last source #1400

lance6716 · 2021-01-26T02:30:59Z

What problem does this PR solve?

What is changed and how it works?

Purge relay log directory when switch to different upstream.

If reconnected to same upstream, relay will look at position saved in relay meta and needed position which specified by user or saved in syncer checkpoint. If relay.meta is left behind, purge the relay folder

Check List

Tests

Integration test
Manual test (check in changed integration test, relay-dir has indeed contained "mysql-bin.000001")

Code changes

Side effects

Related changes

Need to cherry-pick to the release branch
Need to be included in the release note

lance6716 · 2021-01-27T05:58:18Z

PTAL @3pointer @lichunzhu

lance6716 · 2021-01-28T05:03:15Z

Done. But if stop source when worker is offline, need to manually purge relay-dir

now we should check if a source is deleted, changed some information like 'relay-binlog-gtid' and restarted, DM should handle it properly.

maybe (*realRelayHolder).stopRelay or other logic should remove relay-dir.

…O MASTER_AUTO_POSITION = 1, but the master has purged binary logs containing GTIDs that the slave requires.

GMHDBJD · 2021-01-28T15:53:24Z

worker1<-source1   crash
worker2<-source1   crash
worker1<-source1   start pull from max(checkpoint, recoverd relay meta)'s binlog

seems this pr will always start pull the binlog files from relay.meta?

lance6716 · 2021-01-28T23:58:28Z

worker1<-source1   crash
worker2<-source1   crash
worker1<-source1   start pull from max(checkpoint, recoverd relay meta)'s binlog
seems this pr will always start pull the binlog files from relay.meta?

yes, I want to ensure relay log is complete. if upstream has purged the binlog at the position of relay.meta, I am expecting DM auto switch to remote binlog.

lichunzhu · 2021-01-28T09:20:26Z

dm/worker/relay.go

+	// purge relay dir when delete source
+	if err := h.relay.PurgeRelayDir(); err != nil {
+		return err
+	}
+


I think it's a bit aggressive.

how to change 🤔

I suggest moving relay dir to another name (relay_dir -> relay_dir_deprecated) instead of deleting it directly. We have purge-relay command and I think the purge action should only happen in this command.

relay_dir_deprecated is out of relay_dir thus can't be deleted using purge-relay. And disk space is not freed

I don't think it's a good idea. stop source is a normal operation. shouldn't purge data or move to another directory.

dm/worker/server.go

relay/relay.go

tests/relay_interrupt/run.sh

GMHDBJD · 2021-01-29T04:25:17Z

yes, I want to ensure relay log is complete. if upstream has purged the binlog at the position of relay.meta, I am expecting DM auto switch to remote binlog.

Haven't implemented now? I'm afraid that it can't meet user expectations and hard to locate the error since behavior has changed. Moreover, sync should wait relay for a long time if it start from the meta pos but not checkpoint.

lance6716 · 2021-01-29T04:29:57Z

yes, I want to ensure relay log is complete. if upstream has purged the binlog at the position of relay.meta, I am expecting DM auto switch to remote binlog.

Haven't implemented now? I'm afraid that it can't meet user expectations and hard to locate the error since behavior has changed. Moreover, sync should wait relay for a long time if it start from the meta pos but not checkpoint.

the tests/relay_interrupt/run.sh changed in this PR contains the binlog type switching test, so I think it's implemented.

yes, sync will wait a longer time. My prefer for a complete relay log is to handle this case: if an upstream/worker handles multiple tasks, later added tasks may access the hole of relay log thus lost events

GMHDBJD · 2021-01-29T05:51:09Z

the tests/relay_interrupt/run.sh changed in this PR contains the binlog type switching test, so I think it's implemented.

That happens when sync meet errors, not relay. If relay meet an error, like binlog has been purged, sync will block now.

yes, sync will wait a longer time. My prefer for a complete relay log is to handle this case: if an upstream/worker handles multiple tasks, later added tasks may access the hole of relay log thus lost events

I prefer to check the checkpoint and relay meta. When worker bound to a source, compare the min checkpoint of the source and the relay meta location. If checkpoint is larger, purge relay dir and start from checkpoint(This is same as we specify relay-binlog-pos/gtid for a new source), if checkpoint is smaller, pull binlog from relay meta location.

If user add a new task for that source which older than relay, we just report an error. That's same as user specify relay-binlog-pos/gtid, but start from a older location in syncer, or sync start from a location which has been purged in relay.

lance6716 · 2021-01-29T07:27:25Z

That happens when sync meet errors, not relay. If relay meet an error, like binlog has been purged, sync will block now.

I think we should also switch to remote binlog when relay meets error?

lichunzhu

LGTM

lance6716 · 2021-02-02T06:58:39Z

/run-all-tests

need another LGTM

3pointer

LGTM

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2021-02-04T11:13:54Z

cherry pick to release-2.0 in PR #1428

relay: don't purge relay logs when connected to last source

dadb4a3

lance6716 added the status/WIP This PR is still work in progress label Jan 26, 2021

lance6716 added 5 commits January 26, 2021 14:56

fix behaviour

3b4d35f

Merge branch 'master' into dont-purge

d426a66

fix test

9df48b7

add some tests

83a0f82

Merge branch 'master' of https://github.com/pingcap/dm into dont-purge

4c7a9cb

lance6716 changed the title ~~[WIP] relay: don't purge relay logs when connected to last source~~ relay: don't purge relay logs when connected to last source Jan 27, 2021

Merge branch 'master' into dont-purge

ead923d

lance6716 added status/DNM Do not merge, test is failing or blocked by another PR and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jan 28, 2021

lance6716 added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/DNM Do not merge, test is failing or blocked by another PR labels Jan 28, 2021

lance6716 added 3 commits January 28, 2021 15:39

fix ERROR 1236 (HY000): The slave is connecting using CHANGE MASTER T…

10543db

…O MASTER_AUTO_POSITION = 1, but the master has purged binary logs containing GTIDs that the slave requires.

Merge branch 'dont-purge' of github.com:lance6716/dm into dont-purge

08cd1a9

purge relay-dir when stop source

6f12b59

lichunzhu reviewed Jan 29, 2021

View reviewed changes

lichunzhu and others added 3 commits January 28, 2021 21:57

Merge branch 'master' into dont-purge

34b58aa

address comments and move some logic

e0b20a6

Merge branch 'dont-purge' of github.com:lance6716/dm into dont-purge

7ffc952

address comment

16d7c07

lance6716 added 3 commits January 29, 2021 15:27

Merge branch 'master' into dont-purge

88656cd

remove the behaviour "purge relay when stop source"; add test

379151d

move ResetStart into interface

4de7e74

lance6716 force-pushed the dont-purge branch from 9829a85 to 4de7e74 Compare February 2, 2021 06:12

Merge branch 'master' into dont-purge

4bed56b

lichunzhu previously approved these changes Feb 2, 2021

View reviewed changes

lance6716 added the status/LGT1 One reviewer already commented LGTM label Feb 2, 2021

lance6716 added this to the v2.0.2 milestone Feb 2, 2021

3pointer reviewed Feb 4, 2021

View reviewed changes

3pointer approved these changes Feb 4, 2021

View reviewed changes

Merge branch 'master' into dont-purge

65acc5c

lance6716 merged commit efa5b30 into pingcap:master Feb 4, 2021

ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Feb 4, 2021

cherry pick pingcap#1400 to release-2.0

2ae4052

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Feb 4, 2021

relay: don't purge relay logs when connected to last source (#1400) #1428

Merged

lance6716 pushed a commit that referenced this pull request Feb 5, 2021

cherry pick #1400 to release-2.0 (#1428)

3b06a77

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

relay: don't purge relay logs when connected to last source #1400

relay: don't purge relay logs when connected to last source #1400

lance6716 commented Jan 26, 2021 •

edited

Loading

lance6716 commented Jan 27, 2021

lance6716 commented Jan 28, 2021 •

edited

Loading

GMHDBJD commented Jan 28, 2021

lance6716 commented Jan 28, 2021

lichunzhu Jan 28, 2021

lance6716 Jan 29, 2021

lichunzhu Jan 29, 2021 •

edited

Loading

lance6716 Jan 29, 2021

3pointer Jan 29, 2021

GMHDBJD commented Jan 29, 2021

lance6716 commented Jan 29, 2021 •

edited

Loading

GMHDBJD commented Jan 29, 2021 •

edited

Loading

lance6716 commented Jan 29, 2021

lichunzhu left a comment

lance6716 commented Feb 2, 2021

3pointer left a comment •

edited

Loading

ti-srebot commented Feb 4, 2021

relay: don't purge relay logs when connected to last source #1400

relay: don't purge relay logs when connected to last source #1400

Conversation

lance6716 commented Jan 26, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

lance6716 commented Jan 27, 2021

lance6716 commented Jan 28, 2021 • edited Loading

GMHDBJD commented Jan 28, 2021

lance6716 commented Jan 28, 2021

lichunzhu Jan 28, 2021

Choose a reason for hiding this comment

lance6716 Jan 29, 2021

Choose a reason for hiding this comment

lichunzhu Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

lance6716 Jan 29, 2021

Choose a reason for hiding this comment

3pointer Jan 29, 2021

Choose a reason for hiding this comment

GMHDBJD commented Jan 29, 2021

lance6716 commented Jan 29, 2021 • edited Loading

GMHDBJD commented Jan 29, 2021 • edited Loading

lance6716 commented Jan 29, 2021

lichunzhu left a comment

Choose a reason for hiding this comment

lance6716 commented Feb 2, 2021

3pointer left a comment • edited Loading

Choose a reason for hiding this comment

ti-srebot commented Feb 4, 2021

lance6716 commented Jan 26, 2021 •

edited

Loading

lance6716 commented Jan 28, 2021 •

edited

Loading

lichunzhu Jan 29, 2021 •

edited

Loading

lance6716 commented Jan 29, 2021 •

edited

Loading

GMHDBJD commented Jan 29, 2021 •

edited

Loading

3pointer left a comment •

edited

Loading