Simplify write thread logic #2134

yiwu-arbug · 2017-04-11T18:50:16Z

Summary:
The concept about early exit in write thread implementation is a confusing one. It means that if early exit is allowed, batch group leader will not responsible to exit the batch group, but the last finished writer do. In case we need to mark log synced, or encounter memtable insert error, early exit is disallowed.

This patch remove such a concept by:

In all cases, the last finished writer (not necessary leader) is responsible to exit batch group.
In case of parallel memtable write, leader will also mark log synced after memtable insert and before signal finish (call CompleteParallelWorker()). The purpose is to allow mark log synced (which require locking mutex) can run in parallel to memtable insert in other writers.
The last finish writer should handle memtable insert error (update bg_error_) before exiting batch group.

Test Plan:
existing tests.

yiwu-arbug · 2017-04-11T18:51:27Z

@al13n321 adding you to have one more eye on the logic, since you initially add the logic around log sync synchronization.

facebook-github-bot · 2017-04-11T18:51:40Z

@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

maysamyabandeh · 2017-04-13T16:01:44Z

db/db_impl_write.cc

@@ -170,15 +173,13 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options,
      }
    }

-    uint64_t last_sequence = versions_->LastSequence();


What is the benefit of moving up the definition of last_sequence for 50 lines? Is not it better to delay the definition until the variable is actually used? In this way a code reader has less logic to keep in mind when reading the code.

I know in the classic programming it was suggested to put all the variable definitions together but in such a long function i do not see much benefit.

Moving simply because I need last_sequence when I call versions_->SetLastSequence at the end.

maysamyabandeh · 2017-04-13T16:16:58Z

db/db_impl_write.cc

@@ -213,26 +214,18 @@ Status DBImpl::WriteImpl(const WriteOptions& write_options,
      PERF_TIMER_GUARD(write_memtable_time);

      if (!parallel) {
-        status = WriteBatchInternal::InsertInto(
+        w.status = WriteBatchInternal::InsertInto(


What is the reason for this change?

maysamyabandeh · 2017-04-13T16:21:40Z

db/db_impl_write.cc

-      //
-      // Is setting bg_error_ enough here?  This will at least stop
-      // compaction and fail any further writes.
-      if (!status.ok() && !w.CallbackFailed()) {


The check for CallbackFailed is not done when calling UpdateBackgroundError. Can you explain why?

if callback fail we should never try insert into memtable. I call UpdateBackgroundError only for memtable insert error. Any suggestion to renaming/change of logic?

maysamyabandeh · 2017-04-13T16:23:45Z

db/db_impl_write.cc

      }
    }
  }
  PERF_TIMER_START(write_pre_and_post_process_time);

+  //
+  // Is setting bg_error_ enough here?  This will at least stop


This comment is move from another location where we set bg_error_ independent of paranoid_check status. Can you explain more what it means and why it made sense to move it down here?

I should move it to insideUpdateBackgroundError.

maysamyabandeh · 2017-04-13T16:29:11Z

db/db_impl_write.cc

-        // CompleteParallelWorker returns true if this thread should
-        // handle exit, false means somebody else did
-        exit_completed_early = !write_thread_.CompleteParallelWorker(&w);
-        status = w.FinalStatus();


Here we always set status whether or not its current value is ok. However later in Line 277 we set it only if status.ok(). This two behavior does not seem equivalent to me.

maysamyabandeh · 2017-04-13T16:29:48Z

db/db_impl_write.cc

    write_thread_.ExitAsBatchGroupLeader(&w, last_writer, w.status);
  }

+  if (status.ok()) {


Refer to my comment at Line 253 where status is updated independent of its current value.

maysamyabandeh · 2017-04-13T16:33:03Z

db/db_impl_write.cc

@@ -326,7 +319,7 @@ Status DBImpl::PreprocessWrite(const WriteOptions& write_options,
  }

  if (UNLIKELY(status.ok() && !bg_error_.ok())) {
-    status = bg_error_;
+    return bg_error_;


Does it make sense to also check the return value of PreprocessWrite and if it is not OK return immediately?

We can. But then there will be one more place we need to have paranoid check.

maysamyabandeh · 2017-04-13T17:07:42Z

db/db_impl_write.cc

      }
    }
  }
  PERF_TIMER_START(write_pre_and_post_process_time);

+  //
+  // Is setting bg_error_ enough here?  This will at least stop
+  // compaction and fail any further writes.
  if (immutable_db_options_.paranoid_checks && !status.ok() &&


This check was previously done by leader AFTER writing to memtable since we would know that leader is the last one finishing when there is an error. In the refactored version i) we do it BEFORE writing to memtable, ii) only the leader does it if it is not the last one finishing. Can you explain more how these two are equivalent?

status previously include memtable insert error, with the patch now it represent any error except memtable insert error. Memtable insert error is merged into status right before return from WriteImpl.

Paranoid check don't include memtable insert error, same as previous.

leader now execute paranoid check in parallel working phase. It is executed no matter whether it is last one finishing.

I agree this introduce new confusion after the patch. Any suggestion to make the logic clearer?

maysamyabandeh

LGTM (assuming that the tests pass)

Summary: The concept about early exit in write thread implementation is a confusing one. It means that if early exit is allowed, batch group leader will not responsible to exit the batch group, but the last finished writer do. In case we need to mark log synced, or encounter memtable insert error, early exit is disallowed. This patch remove such a concept by: * In all cases, the last finished writer (not necessary leader) is responsible to exit batch group. * In case of parallel memtable write, leader will also mark log synced after memtable insert and before signal finish (call `CompleteParallelWorker()`). The purpose is to allow mark log synced (which require locking mutex) can run in parallel to memtable insert in other writers. * The last finish writer should handle memtable insert error (update bg_error_) before exiting batch group. Closes #2134 Differential Revision: D4869667 Pulled By: yiwu-arbug fbshipit-source-id: aec170847c85b90f4179d6a4608a4fe1361544e3

sync_log

ac99770

yiwu-arbug requested review from maysamyabandeh, al13n321 and siying April 11, 2017 18:50

facebook-github-bot added the CLA Signed label Apr 11, 2017

maysamyabandeh reviewed Apr 13, 2017

View reviewed changes

maysamyabandeh approved these changes Apr 13, 2017

View reviewed changes

facebook-github-bot closed this in e9e6e53 Apr 13, 2017

yiwu-arbug deleted the sync_log branch May 17, 2017 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify write thread logic #2134

Simplify write thread logic #2134

yiwu-arbug commented Apr 11, 2017

yiwu-arbug commented Apr 11, 2017

facebook-github-bot commented Apr 11, 2017

maysamyabandeh Apr 13, 2017

yiwu-arbug Apr 13, 2017 •

edited by maysamyabandeh

Loading

maysamyabandeh Apr 13, 2017

maysamyabandeh Apr 13, 2017

yiwu-arbug Apr 13, 2017

maysamyabandeh Apr 13, 2017

yiwu-arbug Apr 13, 2017

maysamyabandeh Apr 13, 2017

maysamyabandeh Apr 13, 2017

maysamyabandeh Apr 13, 2017

yiwu-arbug Apr 13, 2017

maysamyabandeh Apr 13, 2017

yiwu-arbug Apr 13, 2017

maysamyabandeh left a comment

Simplify write thread logic #2134

Simplify write thread logic #2134

Conversation

yiwu-arbug commented Apr 11, 2017

yiwu-arbug commented Apr 11, 2017

facebook-github-bot commented Apr 11, 2017

Choose a reason for hiding this comment

yiwu-arbug Apr 13, 2017 • edited by maysamyabandeh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysamyabandeh left a comment

Choose a reason for hiding this comment

yiwu-arbug Apr 13, 2017 •

edited by maysamyabandeh

Loading