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

lightning: make every engine use a unique commitTS #1101

Merged
merged 12 commits into from
Jun 21, 2021
Merged

lightning: make every engine use a unique commitTS #1101

merged 12 commits into from
Jun 21, 2021

Conversation

sleepymole
Copy link
Contributor

@sleepymole sleepymole commented May 11, 2021

What problem does this PR solve?

close #1108

To support duplicate detection for local backend, we need to ensure a same commitTS is used for the same engine to avoid ingesting kv pairs more than once with different commitTS. And we should make every engine use a unique commitTS so that we can detect cross-engine duplicates

Do not merge this pr before #1080 .

What is changed and how it works?

  • Make every engine use a unique commitTS which is obtained from PD
  • Attach a commitTS to every engine and save it in engineMeta.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has persistent data change

Side effects

  • Increased code complexity

Related changes

Release Note

  • lightning: make every engine use a unique commitTS

@CLAassistant
Copy link

CLAassistant commented May 11, 2021

CLA assistant check
All committers have signed the CLA.

@purelind
Copy link

/run-check-release-note

@purelind
Copy link

/build

@sleepymole
Copy link
Contributor Author

/cc @glorv @kennytm @Little-Wallace

@purelind
Copy link

/run-check-release-note

@purelind
Copy link

/build

@purelind
Copy link

/run-integration-test

@sleepymole
Copy link
Contributor Author

/build

1 similar comment
@purelind
Copy link

/build

@sleepymole
Copy link
Contributor Author

/run-integration-test

@sleepymole
Copy link
Contributor Author

/hold

@sleepymole
Copy link
Contributor Author

/unhold

@glorv
Copy link
Collaborator

glorv commented May 31, 2021

/build

@kennytm
Copy link
Collaborator

kennytm commented Jun 16, 2021

/run-integration-test

Copy link
Collaborator

@kennytm kennytm 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

pkg/lightning/restore/restore.go Outdated Show resolved Hide resolved
pkg/lightning/backend/backend.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the status/LGT1 LGTM1 label Jun 18, 2021
dataSynced = cr.maybeSaveCheckpoint(rc, t, engineID, cr.chunk, dataEngine, indexEngine)
// When enabled DiskQuota, `enforceDiskQuota` may force the backend to import the content of a large engine
// into the target and then reset the engine to empty. When this happen, we need to use a new commitTS.
if err = rc.backend.AllocateTSIfNotExists(ctx, t.tableName, engineID); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not do this in OpenEngine and resetEngine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds very reasonable. Maybe we can do it inside each backend's implementation and remove AllocateTSIfNotExists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 0bb61ab

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • glorv
  • kennytm

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added status/LGT2 LGTM2 and removed status/LGT1 LGTM1 labels Jun 21, 2021
@glorv
Copy link
Collaborator

glorv commented Jun 21, 2021

@kennytm PTAL again since some interface has changed

Copy link
Collaborator

@Little-Wallace Little-Wallace left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm
Copy link
Collaborator

kennytm commented Jun 21, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 0bb61ab

@ti-chi-bot ti-chi-bot merged commit d9766fd into pingcap:master Jun 21, 2021
@sleepymole sleepymole deleted the commit-ts branch June 21, 2021 08:07
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.

lightning: let each engine use a unique commit ts when ingest into tikv
8 participants