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.

Sign up for GitHub

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

Jump to bottom

memdb: add Flush and Discard API #15780

Merged
merged 15 commits into from Mar 31, 2020
Merged

memdb: add Flush and Discard API #15780

merged 15 commits into from Mar 31, 2020

Conversation

bobotu
Copy link
Contributor

@bobotu bobotu commented Mar 27, 2020

What problem does this PR solve?

Because #15405 is too large to submit as a single PR, I'm going to port the new revertable mem buffer part in this PR.

To maintain compatibility, I currently keep the DB type, but it's really just a wrapper for the Sandbox. After the other parts of the code are updated in subsequent PRs, the DB will be deleted.

What is changed and how it works?

The core of the new design is Sandbox. All modifications will be president into root Sandbox. Sandbox can derive a new child Sandbox at any time.

Each Sandbox is an independent skip-list, with all nodes stored in a shared arena.

Flush will use an optimized in-place algorithm to merge all nodes in the current Sandbox to its parent's skip-list.

Discard will simply rollback the arena's state to the snapshot takes at the creation of the derived Sandbox.

Check List

Tests

  • Unit test

@bobotu bobotu added the sig/transaction SIG:Transaction label Mar 27, 2020
@codecov
Copy link

codecov bot commented Mar 30, 2020 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@25f15c7). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #15780   +/-   ##
===========================================
  Coverage          ?   80.8249%           
===========================================
  Files             ?        505           
  Lines             ?     136949           
  Branches          ?          0           
===========================================
  Hits              ?     110689           
  Misses            ?      17802           
  Partials          ?       8458

@bobotu
Copy link
Contributor Author

bobotu commented Mar 30, 2020

/run-all-tests

coocood
coocood reviewed Mar 30, 2020
View reviewed changes
kv/memdb/memdb_test.go Outdated Show resolved Hide resolved
@bobotu
Copy link
Contributor Author

bobotu commented Mar 31, 2020

PTAL @coocood

coocood
coocood reviewed Mar 31, 2020
View reviewed changes
kv/memdb/arena.go Outdated
type arenaSnapshot struct {
blockSize int
blocks int
availIdx int
Copy link
Member

Choose a reason for hiding this comment

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

Seems availIdx is always equal to blocks-1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/bobotu/tidb/blob/14c3dd17e6bd0c5b578eeffe3e1b3c532224f58e/kv/memdb/arena.go#L106

If size larger than maxBlockSize (128M), avaliIdx will less than len(blocks) - 1.

But will such a large value appear in practice? If not, we can remove this part of the logic to simplify the code.

Copy link
Member

Choose a reason for hiding this comment

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

We have a 6MB entry limit, so it does not happen, we can simplify the code.

@bobotu bobotu requested a review from coocood March 31, 2020 07:26
@coocood
Copy link
Member

coocood commented Mar 31, 2020

LGTM

@bobotu
Copy link
Contributor Author

bobotu commented Mar 31, 2020

PTAL @tiancaiamao

@tiancaiamao
Copy link
Contributor

/run-all-tests

@bobotu
Copy link
Contributor Author

bobotu commented Mar 31, 2020

/run-all-tests

1 similar comment
@bobotu
Copy link
Contributor Author

bobotu commented Mar 31, 2020

/run-all-tests

@bobotu
Copy link
Contributor Author

bobotu commented Mar 31, 2020

/run-all-tests

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 31, 2020
@coocood
Copy link
Member

coocood commented Mar 31, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 31, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 31, 2020

/run-all-tests

@sre-bot sre-bot merged commit 9baaaeb into pingcap:master Mar 31, 2020
@bobotu bobotu deleted the sandbox branch March 31, 2020 14:24
@bobotu bobotu mentioned this pull request Mar 31, 2020
Merged
@bobotu
Copy link
Contributor Author

bobotu commented Apr 14, 2020

/run-cherry-picker

1 similar comment
@bobotu
Copy link
Contributor Author

bobotu commented Apr 14, 2020

/run-cherry-picker

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 14, 2020
@sre-bot
memdb: add Flush and Discard API (pingcap#15780)
29c6281
@sre-bot sre-bot mentioned this pull request Apr 14, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Apr 14, 2020

cherry pick to release-4.0 in PR #16347

zz-jason pushed a commit that referenced this pull request Apr 15, 2020
@sre-bot
memdb: add Flush and Discard API (#15780) (#16347)
8f7daad
@sre-bot sre-bot mentioned this pull request Apr 15, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiancaiamao tiancaiamao tiancaiamao approved these changes

@coocood coocood Awaiting requested review from coocood

Assignees
No one assigned
Labels
sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bobotu @coocood @tiancaiamao @sre-bot