Add GetUpdateSince() to crash test by hx235 · Pull Request #12646 · facebook/rocksdb

hx235 · 2024-05-11T22:39:08Z

Context/Summary: as titled

Test: CI [ongoing]

facebook-github-bot · 2024-05-11T22:39:32Z

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

cbi42 · 2024-05-14T23:32:15Z

+
+    last_seqno = res.sequence;
+    iter->Next();
+  }


Can check status after !iter->Valid()

cbi42 · 2024-05-14T23:34:30Z

+  iter->Next();
+
+  while (iter->Valid()) {
+    if (!iter->status().ok()) {


GetUpdateSince() seems to expect WAL to contain updates with consecutive sequence numbers. It can fail the stress test here when there is a concurrent file ingestion: #10007. There may be other incompatibilities like writes with disable_wal can cause a hole in sequence numbers in WAL too (https://groups.google.com/g/rocksdb/c/W1Axka8CVf8).

Thanks - need to look into more of this.

Fixed and launching more tests to check

Found more incompatibility run - need to fix them

cbi42 · 2024-05-14T23:36:46Z

+    if dest_params.get("get_update_since_one_in") != 0:
+        # Set very high values to avoid WAL cleanup during `GetUpdatesSince()`
+        dest_params["WAL_ttl_seconds"] = 0xFFFFFFFFFFFFFFFF
+        dest_params["WAL_size_limit_MB"] = 0xFFFFFFFFFFFFFFFF


Should we still set some limit to avoid running out of space?

Tuning down the parameter will make the GetUpdateSinceOneIn fail in more ways. This is more difficult than I anticipate will take a look

Fixed - it turns out that we don't have to keep such a high value. If WAL was deleted, the returned iterator will just be ok() but not valid.

cbi42 · 2024-05-14T23:37:11Z

    "optimize_filters_for_memory": lambda: random.randint(0, 1),
    "partition_filters": lambda: random.randint(0, 1),
    "partition_pinning": lambda: random.randint(0, 3),
+    "get_update_since_one_in": lambda: random.choice([0, 0, 0, 1000]),


May need to sanity check that disable_wal is 0.

hx235 · 2024-05-17T18:07:54Z

~~Might need to temporarily pause on this as the space vs functionality trade-off is harder to solve than expected. ~~ Got some new ideas from @ajkr so can continue now

facebook-github-bot · 2024-05-17T18:13:48Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-05-23T19:09:48Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-05-23T19:14:16Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-05-23T19:14:31Z

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

facebook-github-bot · 2024-05-23T19:17:30Z

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

facebook-github-bot · 2024-05-24T21:59:55Z

@hx235 has updated the pull request. You must reimport the pull request before landing.

ajkr

Thanks for working on this. I think there are some possible improvements, though

ajkr · 2024-06-06T01:11:23Z

+  // `TryAgain()` is allowed as it might happens when new writes happen between
+  // calling `GetLatestSequenceNumber()` above and `iter->Next()`


Sorry I don't understand. GetLatestSequenceNumber() appears to be the maximum possible value of the lower bound we pass to GetUpdatesSince().

ajkr · 2024-06-06T01:21:02Z

+DEFINE_int32(get_update_since_one_in, 0,
+             "If non-zero, then GetUpdateSince() will be called "


s/get_update_since/get_updates_since/g
s/GetUpdateSince/GetUpdatesSince/g

ajkr · 2024-06-06T01:27:56Z

    "partition_pinning": lambda: random.randint(0, 3),
+    # `get_update_since_one_in` requires no sequence number hole in all WALs
+    # To simplify santization, we enable or disable `get_update_since_one_in` consistently across runs
+    "get_update_since_one_in": random.choice([0, 1000000]),


Since it is a relatively uncommon/simpler function and testing it prevents us from testing common/complicated functions (disableWAL writes, IngestExternalFile()), I think we should enable it much less frequently. Maybe one in ten or even less frequent.

ajkr · 2024-06-06T01:36:03Z

    "partition_filters": lambda: random.randint(0, 1),
    "partition_pinning": lambda: random.randint(0, 3),
+    # `get_update_since_one_in` requires no sequence number hole in all WALs
+    # To simplify santization, we enable or disable `get_update_since_one_in` consistently across runs


Hm that makes db_crashtest.py harder to use, compared to changing it per run.

If we save the GetLatestSequenceNumber() at startup and use that as a lower bound for GetUpdatesSince()'s seq_number, does that allow us to ignore what past runs did?

ajkr · 2024-06-06T01:43:22Z

+  if (!iter->status().ok()) {
+    fprintf(stderr,
+            "Iterator with non-ok status returned by "
+            "GetUpdatesSince(): %s \n",
+            iter->status().ToString().c_str());
+    thread->shared->SafeTerminate();
+  } else if (!iter->Valid()) {
+    return;
+  }
+
+  BatchResult res = iter->GetBatch();
+  SequenceNumber last_seqno = res.sequence;
+  iter->Next();


This seems like a lot of extra code to avoid a special case for the first iteration of the loop. Can we initialize last_seqno to zero and let the loop treat that as a value that does not need consecutive-ness checks?

hx235 · 2024-06-06T04:30:32Z

Won't be able to get to this now since there are some more context for me to learn and stablize the test. Will get to this later.

jsteemann · 2024-06-10T13:12:14Z

+            iter->status().ToString().c_str());
+    thread->shared->SafeTerminate();
+  } else if (!iter->Valid()) {
+    return;


If I am not wrong, then file deletions should be re-enabled here again by calling db_->EnableFileDeletions();.
Otherwise this test would disable file deletions and not turn them back on, so it may influence the behavior of other tests.

Great catch. I think ideally this special case would be deleted and we just let control flow through the end of the function per https://github.com/facebook/rocksdb/pull/12646/files#r1628634984. Still, thanks for demonstrating how seemingly small/innocent extra code can make things subtly wrong.

Good catch! Early return needs to be handled carefully with clean up work…

jowlyzhang · 2024-07-16T19:52:30Z

        # WriteCommitted only
        dest_params["use_put_entity_one_in"] = 0
+    if dest_params.get("get_update_since_one_in") != 0:
+        # To avoid sequence number hole in WAL


I think log file recycling is also not compatible with GetUpdatesSince so maybe we can disable it here too. We should probably mention this in the documentation for GetUpdatesSince.

according to facebook/rocksdb#12646 (comment), GetUpdatesSince() may be incompatible with recycling log files.

facebook-github-bot added the CLA Signed label May 11, 2024

hx235 requested review from ajkr, cbi42 and jowlyzhang May 13, 2024 19:48

cbi42 reviewed May 14, 2024

View reviewed changes

hx235 closed this May 17, 2024

hx235 reopened this May 17, 2024

hx235 force-pushed the get_update_since branch from 4c380dc to 7647a4b Compare May 17, 2024 18:13

hx235 force-pushed the get_update_since branch from 7647a4b to 351ae10 Compare May 23, 2024 19:09

hx235 force-pushed the get_update_since branch from 351ae10 to f663e83 Compare May 23, 2024 19:14

add

19ac8a0

hx235 force-pushed the get_update_since branch from f663e83 to 19ac8a0 Compare May 24, 2024 21:59

ajkr suggested changes Jun 6, 2024

View reviewed changes

jsteemann reviewed Jun 10, 2024

View reviewed changes

jowlyzhang reviewed Jul 16, 2024

View reviewed changes

jsteemann added a commit to arangodb/arangodb that referenced this pull request Jul 22, 2024

revert options change according

429e0f4

according to facebook/rocksdb#12646 (comment), GetUpdatesSince() may be incompatible with recycling log files.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add GetUpdateSince() to crash test#12646

Add GetUpdateSince() to crash test#12646
hx235 wants to merge 1 commit into
facebook:mainfrom
hx235:get_update_since

hx235 commented May 11, 2024 •

edited

Loading

facebook-github-bot commented May 11, 2024

cbi42 May 14, 2024

hx235 May 23, 2024

cbi42 May 14, 2024 •

edited

Loading

hx235 May 14, 2024

hx235 May 23, 2024

hx235 May 24, 2024

cbi42 May 14, 2024

hx235 May 17, 2024

hx235 May 23, 2024 •

edited

Loading

cbi42 May 14, 2024

hx235 May 23, 2024

hx235 commented May 17, 2024 •

edited

Loading

facebook-github-bot commented May 17, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 24, 2024

ajkr left a comment

ajkr Jun 6, 2024

ajkr Jun 6, 2024

ajkr Jun 6, 2024

ajkr Jun 6, 2024

ajkr Jun 6, 2024

hx235 commented Jun 6, 2024

jsteemann Jun 10, 2024

ajkr Jun 10, 2024

hx235 Jun 10, 2024

jowlyzhang Jul 16, 2024

Labels

6 participants

		// `TryAgain()` is allowed as it might happens when new writes happen between
		// calling `GetLatestSequenceNumber()` above and `iter->Next()`

		DEFINE_int32(get_update_since_one_in, 0,
		"If non-zero, then GetUpdateSince() will be called "

Uh oh!

Conversation

hx235 commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

facebook-github-bot commented May 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbi42 May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

hx235 May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

facebook-github-bot commented May 17, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 24, 2024

ajkr 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

hx235 commented Jun 6, 2024

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

Labels

6 participants

hx235 commented May 11, 2024 •

edited

Loading

cbi42 May 14, 2024 •

edited

Loading

hx235 May 23, 2024 •

edited

Loading

hx235 commented May 17, 2024 •

edited

Loading