Option to fail a request as incomplete when skipping too many internal keys #2000

sagar0 · 2017-03-17T18:56:54Z

Operations like Seek/Next/Prev sometimes take too long to complete when there are many internal keys to be skipped. Adding an option, max_skippable_internal_keys -- which could be used to set a threshold for the maximum number of keys that can be skipped, will help to address these cases where it is much better to fail a request (as incomplete) than to wait for a considerable time for the request to complete.

This feature -- to fail an iterator seek request as incomplete, is disabled by default when max_skippable_internal_keys = 0. It is enabled only when max_skippable_internal_keys > 0.

This feature is based on the discussion mentioned in the PR #1084.

siying · 2017-03-17T20:24:30Z

@sagar0 make sure you add an entry in HISTORY.md. That will convert to release notes.

sagar0 · 2017-03-17T20:26:03Z

@siying sure, will do.

facebook-github-bot · 2017-03-22T07:18:00Z

@sagar0 updated the pull request - view changes

facebook-github-bot · 2017-03-22T07:34:17Z

@sagar0 updated the pull request - view changes

facebook-github-bot · 2017-03-22T07:44:43Z

@sagar0 updated the pull request - view changes

facebook-github-bot · 2017-03-22T07:47:34Z

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

lightmark · 2017-03-18T00:02:17Z

db/db_iter.cc

+    } else {
+      num_tombstones_skipped_++;
+    }
+


Since the option name is max_tombstones_skip_in_iterations, as discussed offline, we should have a better name for this since it includes every iterator no matter whether it is tombstone or not.

lightmark · 2017-03-18T00:07:21Z

db/db_iter.cc

+    //  } else {
+    //    num_tombstones_skipped_++;
+    //  }
+


Same here, which should be moved to line 769 I think.

lightmark · 2017-03-22T23:15:30Z

db/db_iter.cc

+    } else {
+      num_internal_keys_skipped_++;
+    }
+


I think this part should be on line 789. The first time we enter this loop we don't know whether the key will be skipped.

Also, how to deal with too many merge_operands in this case?

That's a reasonable suggestion. I believe I added this code snippet here as the other check (in line 742) is also here, and I wanted to keep both of them close together.

@lightmark should there be something different done for merges? I am assuming that they should also be handled in the same way. We should still fail and return an incomplete status if too many keys are being looked for getting the value of a merge. Thoughts?

This sounds like a reasonable behavior for merge operands

lightmark · 2017-03-22T23:27:14Z

db/db_iter.cc

+    if (TooManyInternalKeysSkipped()) {
+      return;
+    }
+


These lines are not necessary since you set valid_ = false; already in TooManyInternalKeysSkipped. So line 700 will break;

If I understand the code correctly, valid_'s value need not always be the same as iter_->Valid() value. In this code snippet, if valid_ is set to false in TooManyInternalKeysSkipped(), we will not break in 701 as we never enter the if block (iter_->Valid() need not be false). Hence a return here.
In fact, some of my unit tests check this scenario.

You are right. I thought it was Valid() not iter_->Valid()

lightmark · 2017-03-22T23:29:29Z

db/db_iter.cc

+  }
+  return false;
+}
+


maybe if would be better to move num_internal_keys_skipped_++; before line 969.
Then you can do this instead an if...else...

if (TooManyInternalKeysSkipped()) { return; }

I definitely did consider this, and I would have loved to done the exact same thing which you suggested, but it will not work in the reverse iteration flow as we will end up double counting in a few cases.
The Reason being:
TooManyInternalKeysSkipped is called from both PrevInternal and FindValueForCurrentKey. FindValueForCurrentKey is in turn called from PrevInternal. If the counter is incremented in FindValueForCurrentKey, it should not be incremented again in PrevInternal. One of my unit tests ran into this problem of double counting, and I had to fix it.

Sure. This comment has the prerequisite that the first comment is valid.

lightmark

LGTM. Just consider how to cope with merge in FindValueForCurrentKey

lightmark · 2017-03-23T17:43:14Z

db/db_iter.cc

+    if (TooManyInternalKeysSkipped()) {
+      return;
+    }
+


You are right. I thought it was Valid() not iter_->Valid()

lightmark · 2017-03-23T17:48:30Z

db/db_iter.cc

+  }
+  return false;
+}
+


Sure. This comment has the prerequisite that the first comment is valid.

IslamAbdelRahman · 2017-03-28T21:43:33Z

db/db_iter.cc

@@ -106,7 +106,8 @@ class DBIter: public Iterator {
         uint64_t max_sequential_skip_in_iterations, uint64_t version_number,
         const Slice* iterate_upper_bound = nullptr,
         bool prefix_same_as_start = false, bool pin_data = false,
-         bool total_order_seek = false)
+         bool total_order_seek = false,
+         uint64_t max_skippable_internal_keys = 0)


We keep passing new arguments to the Iterator, I think we should simply pass the ReadOptions. this should be a refactor that we should consider to do in the future

Addressed in #2116.

IslamAbdelRahman · 2017-03-29T01:19:30Z

db/db_iter.cc

@@ -390,6 +400,12 @@ void DBIter::FindNextUserEntryInternal(bool skipping, bool prefix_check) {
      break;
    }

+    if (TooManyInternalKeysSkipped()) {


let's look into if it's possible to have it look like this

if (BumpInternalKeysSkipped()) { return false; }

we can even have a statistics counter inside this function that will report the total number of internal keys that we need to skip

Moved counter-incrementing to be inside the function.
I have not added statistics counters in this PR, but I'll look at adding them in a separate PR.

IslamAbdelRahman · 2017-03-29T01:23:05Z

db/db_iter.cc

+    } else {
+      num_internal_keys_skipped_++;
+    }
+


This sounds like a reasonable behavior for merge operands

facebook-github-bot · 2017-03-29T23:45:24Z

@sagar0 updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-03-29T23:49:49Z

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

IslamAbdelRahman

LGTM, let's wait for the tests to pass then land it

sagar0 requested review from IslamAbdelRahman and siying March 17, 2017 18:56

facebook-github-bot added the CLA Signed label Mar 17, 2017

siying requested review from lightmark and ajkr March 17, 2017 19:42

sagar0 changed the title ~~Option to fail a request as incomplete when skipping too many tombstoned keys~~ Option to fail a request as incomplete when skipping too many internal keys Mar 22, 2017

sagar0 added 14 commits March 22, 2017 00:36

Fail a request as Incomplete if too many tombstones are encountered

e038380

Add max_tombstones_skip_in_iterations option

68417e5

Update the C API

a39d25e

Update the internal iterators to pass max_tombstones option value

f4a1a56

Add skip tombstones option to example options file

8abd8a2

Update db_impl to support the new option for iterators

5898cc3

Fail a request as incomplete if tombstones > threshold

5b8aa33

Handle reverse iteration

502a9f6

Move the new option into ReadOptions

26dd7ce

Rename the option

d9f7079

Fix the user visible status message

cb3c06d

Add more unit tests

0e2e71e

Add the new option to HISTORY.md

9a769ee

make format

8311534

sagar0 force-pushed the max-tombstones branch from 403dc35 to 8311534 Compare March 22, 2017 07:44

lightmark suggested changes Mar 22, 2017

View reviewed changes

lightmark approved these changes Mar 23, 2017

View reviewed changes

IslamAbdelRahman reviewed Mar 29, 2017

View reviewed changes

Move counter incrementing inside TooManyInternalKeysSkipped fn

1b2d09e

IslamAbdelRahman approved these changes Mar 30, 2017

View reviewed changes

facebook-github-bot closed this in c6d04f2 Mar 30, 2017

sagar0 deleted the max-tombstones branch April 4, 2017 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to fail a request as incomplete when skipping too many internal keys #2000

Option to fail a request as incomplete when skipping too many internal keys #2000

sagar0 commented Mar 17, 2017 •

edited

Loading

siying commented Mar 17, 2017

sagar0 commented Mar 17, 2017

facebook-github-bot commented Mar 22, 2017

facebook-github-bot commented Mar 22, 2017

facebook-github-bot commented Mar 22, 2017

facebook-github-bot commented Mar 22, 2017

lightmark Mar 18, 2017

lightmark Mar 18, 2017

lightmark Mar 22, 2017

lightmark Mar 22, 2017

sagar0 Mar 23, 2017

sagar0 Mar 24, 2017

IslamAbdelRahman Mar 29, 2017

lightmark Mar 22, 2017

sagar0 Mar 23, 2017

lightmark Mar 23, 2017

lightmark Mar 22, 2017

sagar0 Mar 23, 2017

lightmark Mar 23, 2017

lightmark left a comment

lightmark Mar 23, 2017

lightmark Mar 23, 2017

IslamAbdelRahman Mar 28, 2017

sagar0 Jul 7, 2017

IslamAbdelRahman Mar 29, 2017

sagar0 Mar 29, 2017

IslamAbdelRahman Mar 29, 2017

facebook-github-bot commented Mar 29, 2017

facebook-github-bot commented Mar 29, 2017

IslamAbdelRahman left a comment

Option to fail a request as incomplete when skipping too many internal keys #2000

Option to fail a request as incomplete when skipping too many internal keys #2000

Conversation

sagar0 commented Mar 17, 2017 • edited Loading

siying commented Mar 17, 2017

sagar0 commented Mar 17, 2017

facebook-github-bot commented Mar 22, 2017

facebook-github-bot commented Mar 22, 2017

facebook-github-bot commented Mar 22, 2017

facebook-github-bot commented Mar 22, 2017

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

Choose a reason for hiding this comment

lightmark left a 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

facebook-github-bot commented Mar 29, 2017

facebook-github-bot commented Mar 29, 2017

IslamAbdelRahman left a comment

Choose a reason for hiding this comment

sagar0 commented Mar 17, 2017 •

edited

Loading