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

owner: add lock control in owner table info writer #154

Merged
merged 7 commits into from
Dec 5, 2019
Merged

owner: add lock control in owner table info writer #154

merged 7 commits into from
Dec 5, 2019

Conversation

amyangfei
Copy link
Contributor

What problem does this PR solve?

When owner add or remove a table to subchangefeedinfo in etcd, it should concern the table-p-lock.
Owner also needs to guarantee local SubChangeFeedInfo is up to date before writing new data into etcd.

What is changed and how it works?

  • If table-p-lock is not resolved, wait for it resolved.
  • If table-p-lock is resolved, remove both table-p-lock and table-c-lock from etcd.

Check List

Tests

  • Unit test
  • Integration test

@amyangfei amyangfei added the status/ptal Could you please take a look? label Dec 3, 2019
@july2993
Copy link
Contributor

july2993 commented Dec 4, 2019

pls resolve conflict

@july2993 july2993 requested a review from suzaku December 4, 2019 07:53
if writePLock {
newInfo.TablePLock = &model.TableLock{
Ts: oracle.EncodeTSO(time.Now().UnixNano() / int64(time.Millisecond)),
CreatorID: util.CaptureIDFromCtx(ctx),
Copy link
Contributor

Choose a reason for hiding this comment

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

I will prefer explicitly param here, but ok with this too now.

Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

LGTM

@july2993 july2993 added LGT1 and removed status/ptal Could you please take a look? labels Dec 4, 2019
@july2993
Copy link
Contributor

july2993 commented Dec 4, 2019

@suzaku @leoppro PTAL

cdc/owner.go Outdated
subInfo.RemoveTable(id)

newInfo, err := c.infoWriter.Write(ctx, c.ID, captureID, subInfo, true)
c.ProcessorInfos[captureID] = newInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

Only set it when err != nil? If newInfo is nil on error, later calls to restoreTableInfos may end in runtime error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, PTAL

Copy link
Contributor

@suzaku suzaku left a comment

Choose a reason for hiding this comment

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

LGTM

@suzaku suzaku added LGT2 and removed LGT1 labels Dec 5, 2019
@amyangfei amyangfei merged commit c6692f1 into pingcap:master Dec 5, 2019
@amyangfei amyangfei deleted the remove-table-using-lock branch December 5, 2019 06:38
5kbpers pushed a commit to 5kbpers/ticdc that referenced this pull request Aug 24, 2020
amyangfei pushed a commit to amyangfei/tiflow that referenced this pull request May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants