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

Fix life time of memtable_write_group #171

Merged
merged 3 commits into from
Apr 28, 2020

Conversation

Little-Wallace
Copy link

@Little-Wallace Little-Wallace commented Apr 27, 2020

I created memtable_write_group in a wrong position, so it was dropped before this thread began running STATE_PARALLEL_MEMTABLE_WRITER . If there are more than one thread trying to insert keys to RocksDB and every of them have more than one WriteBatch, the process will crash because of accessing an invalid memory address.

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@@ -215,6 +215,7 @@ Status DBImpl::MultiBatchWriteImpl(const WriteOptions& write_options,
// and it would not notify the threads in this WriteGroup. So we must make someone in
// this WriteGroup to complete it and leader thread is easy to be decided.
if (is_leader_thread) {
assert(write_thread_.CompleteParallelMemTableWriter(&writer));
Copy link
Collaborator

@yiwu-arbug yiwu-arbug Apr 28, 2020

Choose a reason for hiding this comment

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

From what I remember assert will be optimize out in release build. Do something like the following:

bool ok __attribute__((__unused__)) = write_thread_.CompleteParallelMemTableWriter(&writer);
assert(ok);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused. Without calling CompleteParallelMemTableWriter before, how do the leader make sure all other parallel writers in the group has finished at this point?

Copy link
Author

Choose a reason for hiding this comment

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

Because leader_thread will be blocked in the loop until all followers have finished their tasks.

Copy link
Author

Choose a reason for hiding this comment

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

So in release build, we do not need to run CompleteParallelMemTableWriter.

Copy link
Collaborator

@yiwu-arbug yiwu-arbug Apr 28, 2020

Choose a reason for hiding this comment

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

I prefer calling CompleteParallelMemTableWriter anyway, and return error if the call says the leader is not the last writer to exit from the group. That's the contract with WriteThread.

Copy link
Author

Choose a reason for hiding this comment

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

Ok.

@yiwu-arbug
Copy link
Collaborator

Please describe in the summary what's the issue without the change. Thanks.

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

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

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

LGTM

@Little-Wallace Little-Wallace merged commit 991bb28 into tikv:6.4.tikv Apr 28, 2020
Little-Wallace added a commit that referenced this pull request Apr 30, 2020
* pass enable_multi_thread_write to BuildDBOptions (#170)

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

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

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@Little-Wallace Little-Wallace deleted the fix-batch branch August 26, 2020 07:17
@tabokie tabokie mentioned this pull request May 9, 2022
39 tasks
tabokie pushed a commit to tabokie/rocksdb that referenced this pull request May 12, 2022
* fix life time of memtable_write_group

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
* 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
* 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>
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.

2 participants