Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

Log fold is unstable when doing create and drop table operation and thus affect br restore using cdc log #739

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

yuqi1129
Copy link
Contributor

What problem does this PR solve?

see https://github.com/pingcap/ticdc/issues/1415

What is changed and how it works?

Check List

Tests

  • Unit 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

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release Note

…hus affect br restore using cdc log, fix bug again
@3pointer
Copy link
Collaborator

3pointer commented Mar 9, 2021

I'm afraid this PR will introduce a new issue. I think the proper way to fix it is to choose the proper table id directory with given start-ts and end-ts(not largest table id directory always) in collectRowChangeFiles.

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Mar 9, 2021 via email

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Mar 10, 2021

@3pointer

	nameIdsMap := l.GetNameIDMap()

	log.Debug("nameIdsMap", zap.Any("name and ids:", nameIdsMap))
	for _, ids := range nameIdsMap {
		for _, tID := range ids {
			tableID := tID
			// FIXME update log meta logic here
			dir := fmt.Sprintf("%s%d", tableLogPrefix, tableID)
			opt := &storage.WalkOption{
				SubDir:    dir,
				ListCount: -1,
			}
			err := l.restoreClient.storage.WalkDir(ctx, opt, func(path string, size int64) error {
				fileName := filepath.Base(path)
				shouldRestore, err := l.NeedRestoreRowChange(fileName)
				if err != nil {
					return errors.Trace(err)
				}
				if shouldRestore {
					rowChangeFiles[tableID] = append(rowChangeFiles[tableID], path)
				}
				return nil
			})
			if err != nil {
				return nil, errors.Trace(err)
			}
		}
	}

Hi, The code thats follows l.GetNameIDMap() will traverse cdc storage directory and will choose proper ids thats contains the start time and end time, so i think there is no need to add logic to filter the files, what's your opinion?

@kennytm
Copy link
Collaborator

kennytm commented Mar 12, 2021

/build

@tisonkun
Copy link

@kennytm @3pointer I guess this PR should migrate to pingcap/tidb now? Does it go in a right direction so that we can continue it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants