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

Commit pipeline when write a WriteBatch for linearizability #267

Merged
merged 4 commits into from
Mar 17, 2022
Merged

Commit pipeline when write a WriteBatch for linearizability #267

merged 4 commits into from
Mar 17, 2022

Conversation

Little-Wallace
Copy link

This method is proposed by pebble

I support async method so that the write thread does not need wait this writebatch committed.

@Little-Wallace
Copy link
Author

/test

@Little-Wallace Little-Wallace changed the title [WIP] Commit pipeline when write a WriteBatch for linearizability Commit pipeline when write a WriteBatch for linearizability Mar 2, 2022
@Little-Wallace
Copy link
Author

Little-Wallace commented Mar 2, 2022 •
edited
Loading

See benchmark result in https://docs.google.com/document/d/1s5Dj02hhe18zzwKTUZF4n1Bi0z6p5_d9TfoMkHKm2XE/edit

@Connor1996 Connor1996 self-requested a review March 8, 2022 07:44
Connor1996
Connor1996 reviewed Mar 13, 2022
View reviewed changes
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

https://github.com/tikv/rocksdb/blob/336f6ab917c9861b16b8b1b0a4b0c3f70dcd1fb1/db/db_impl/db_impl_write.cc#L148-L149

Should check writer.status.ok()

Connor1996
Connor1996 reviewed Mar 13, 2022
View reviewed changes
db/db_impl/db_impl_write.cc
assert(writer.state == WriteThread::STATE_COMPLETED);
TEST_SYNC_POINT("DBImpl::WriteImpl:CommitAfterWriteWAL");

if (writer.request->commit_lsn != 0 && writer.status.ok()) {
Copy link
Member

@Connor1996 Connor1996 Mar 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

How about writing like this, it's clearer

if (write.request -> commit_lsn == 0) {
  // 
} else if (write.status.ok()) {
  // 
} else {

}

Copy link
Author

Choose a reason for hiding this comment

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

I do not think so....

Connor1996
Connor1996 reviewed Mar 13, 2022
View reviewed changes
db/write_thread.h Outdated

private:
std::mutex commit_mu_;
std::condition_variable cv_;
Copy link
Member

Choose a reason for hiding this comment

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

use InstrumentedCondVar?

Copy link
Author

Choose a reason for hiding this comment

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

It seems heavily...

Copy link
Member

@tabokie tabokie Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You can use port::Mutex and port::CondVar. Honestly I don't know why the old code uses std::mutex.

@Connor1996 Connor1996 requested a review from tabokie March 13, 2022 15:09
Connor1996
Connor1996 reviewed Mar 13, 2022
View reviewed changes
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

Please change the base branch to 6.4.tikv, rest LGTM

Connor1996
Connor1996 reviewed Mar 15, 2022
View reviewed changes
db/db_impl/db_impl_write.cc Outdated Show resolved Hide resolved
db/db_impl/db_impl_write.cc Show resolved Hide resolved
tabokie
tabokie reviewed Mar 15, 2022
View reviewed changes
db/db_impl/db_impl.h Outdated Show resolved Hide resolved
db/write_thread.h Show resolved Hide resolved
db/write_thread.h Show resolved Hide resolved
db/write_thread.h Outdated Show resolved Hide resolved
db/write_thread.h Outdated Show resolved Hide resolved
db/write_thread.h Outdated Show resolved Hide resolved
db/db_impl/db_impl_write.cc Outdated Show resolved Hide resolved
db/write_thread.h Show resolved Hide resolved
@Little-Wallace Little-Wallace changed the base branch from tikv-5.2 to 6.4.tikv March 16, 2022 06:16
@Little-Wallace
support commit pipeline
527b170 
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@Little-Wallace
fix name
d53f764 
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@Little-Wallace
refactor writeimpl
664e67d 
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Connor1996
Connor1996 approved these changes Mar 16, 2022
View reviewed changes
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

tabokie
tabokie reviewed Mar 16, 2022
View reviewed changes
Copy link
Member

@tabokie tabokie left a comment

Choose a reason for hiding this comment

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

Rest looks good.

db/db_impl/db_impl_write.cc
WriteThread::WriteGroup wal_write_group;
mutex_.Lock();
if (writer.callback && !writer.callback->AllowWriteBatching()) {
WaitForPendingWrites();
Copy link
Member

Choose a reason for hiding this comment

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

Why different than before?

Copy link
Author

Choose a reason for hiding this comment

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

because we need to make sure that all memtable writes are applied before we checkcallback so that we can see a consistent result from DB

db/write_thread.h Outdated Show resolved Hide resolved
@Little-Wallace
fix variable
76fe681 
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@Little-Wallace
Copy link
Author

/test

@Little-Wallace Little-Wallace merged commit 21712fe into tikv:6.4.tikv Mar 17, 2022
@ti-srebot ti-srebot mentioned this pull request Mar 17, 2022
Closed
This was referenced Mar 17, 2022
Merged
Merged
Merged
@tabokie tabokie mentioned this pull request May 9, 2022
Closed
39 tasks
tabokie pushed a commit to tabokie/rocksdb that referenced this pull request May 12, 2022
@Little-Wallace @tabokie
Commit pipeline when write a WriteBatch for linearizability (tikv#267)
225bff0 
* support commit pipeline

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie added a commit that referenced this pull request May 12, 2022
@tabokie @Little-Wallace
Implement pipelined commit (#267) (#284)
d3e4138 
* add multibatch write into memtable (#131)

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Support write multiple WriteBatch into RocksDB together. These WriteBatch will be assigned sequence number in order and pushed into queue. If a thread is waiting for some state, it could steal some job from work queue.

Signed-off-by: tabokie <xy.tao@outlook.com>

* Improve Multi Batch Write (#154)

* perform like normal pipelined write

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

* pass enable_multi_thread_write to BuildDBOptions (#170)

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

* Fix life time of `memtable_write_group` (#171)

* fix life time of memtable_write_group

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

* Commit pipeline when write a WriteBatch for linearizability (#267)

* support commit pipeline

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

* format

Signed-off-by: tabokie <xy.tao@outlook.com>

* remove useless code

Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: Wallace <bupt2013211450@gmail.com>
tabokie added a commit that referenced this pull request May 25, 2022
@tabokie @Little-Wallace
Implement pipelined commit (#267) (#284)
910417b 
* add multibatch write into memtable (#131)

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>

Support write multiple WriteBatch into RocksDB together. These WriteBatch will be assigned sequence number in order and pushed into queue. If a thread is waiting for some state, it could steal some job from work queue.

Signed-off-by: tabokie <xy.tao@outlook.com>

* Improve Multi Batch Write (#154)

* perform like normal pipelined write

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

* pass enable_multi_thread_write to BuildDBOptions (#170)

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

* Fix life time of `memtable_write_group` (#171)

* fix life time of memtable_write_group

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

* Commit pipeline when write a WriteBatch for linearizability (#267)

* support commit pipeline

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: tabokie <xy.tao@outlook.com>

* format

Signed-off-by: tabokie <xy.tao@outlook.com>

* remove useless code

Signed-off-by: tabokie <xy.tao@outlook.com>

Co-authored-by: Wallace <bupt2013211450@gmail.com>
@tabokie tabokie mentioned this pull request Jul 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Connor1996 Connor1996 Connor1996 approved these changes

@tabokie tabokie tabokie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Little-Wallace @Connor1996 @tabokie