Range deletion memory usage improvements #10048

xy-54321 · 2023-04-25T22:26:15Z

convert system key clear range to point delete
flush CF once number of range deletes is greater than 1000

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

The PR has a description, explaining both the problem and the solution.
The description mentions which forms of testing were done and the testing seems reasonable.
Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

fdb-windows-ci · 2023-04-25T23:13:41Z

Doxense CI Report for Windows 10

Commit ID: 31d80a9
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2023-04-25T23:14:51Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 31d80a9
Duration 0:31:11
Result: ❌ FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-25T23:46:29Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: b02503c
Duration 1:16:17
Result: ❌ FAILED
Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-25T23:56:23Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: b02503c
Duration 1:26:12
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-25T23:58:31Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 6b7902b
Duration 1:32:10
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-26T00:05:40Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: b02503c
Duration 1:35:24
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2023-04-26T00:07:51Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 6b7902b
Duration 1:41:26
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2023-04-26T00:08:30Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 31d80a9
Duration 1:24:53
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-26T00:10:21Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 31d80a9
Duration 1:26:46
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2023-04-26T00:13:58Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 467349c
Duration 1:39:54
Result: ❌ FAILED
Error: Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2023-04-26T00:15:38Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 467349c
Duration 1:41:35
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-26T00:59:18Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 6b7902b
Duration 2:32:51
Result: ❌ FAILED
Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-26T01:34:37Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 467349c
Duration 3:00:32
Result: ❌ FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

saintstack

Looks great. Some comments in below.

saintstack · 2023-04-26T19:41:03Z

fdbclient/ServerKnobs.cpp

 	init( ROCKSDB_WAL_RECOVERY_MODE,                               2 ); // kPointInTimeRecovery, RocksDB default.
 	init( ROCKSDB_TARGET_FILE_SIZE_BASE,                    16777216 ); // 16MB, RocksDB default.
 	init( ROCKSDB_MAX_OPEN_FILES,                              50000 ); // Should be smaller than OS's fd limit.
+	init( ROCKSDB_USE_POINT_DELETE_FOR_SYSTEM_KEYS,            false ); if(isSimulated)ROCKSDB_USE_POINT_DELETE_FOR_SYSTEM_KEYS = deterministicRandom()->coinflip();


Is this option meant to address the OOM we've been seeing? It wasn't happening in system keys I don't think.

Where do these two new options get a bit of commentary? They are a little exotic in their operation. Some commentary on what they are about will help those that come after us I think.

saintstack · 2023-04-26T19:43:08Z

fdbserver/KeyValueStoreRocksDB.actor.cpp

+		wait(kvStore->commit(false));
+	}
+
+	// TODO: flush memtable. The process is expected to OOM.


Will this test fail always w/o the rocksdb fix or ROCKSDB_CF_RANGE_DELETION_LIMIT <= 0?

It will not. Single shard rocksdb doesn't have the flush api.

saintstack · 2023-04-26T19:47:18Z

fdbserver/KeyValueStoreShardedRocksDB.actor.cpp

+
+			// TODO: Disable this once RocksDB is upgraded to a version with range delete improvement.
+			if (SERVER_KNOBS->ROCKSDB_USE_POINT_DELETE_FOR_SYSTEM_KEYS && systemKeys.contains(range)) {
+				rocksdb::ReadOptions options = getReadOptions();


The read options here won't have the issue Neethu just ran into around prefix for blooms? The auto_prefix_mode setting?

yes. I updated the read options.

saintstack · 2023-04-26T19:48:42Z

fdbserver/KeyValueStoreShardedRocksDB.actor.cpp

+				if (!cursor->status().ok()) {
+					// if readrange iteration fails, then do a deleteRange.
+					writeBatch->DeleteRange(physicalShard->cf, beginSlice, endSlice);
+				}


Is this code repeated elsewhere? E.g. The Neethu feature that changes range deletes to point deletes? Can we reuse her implementation?

The main idea is the same. Because we read multiple CFs here, the implementation is quite different.

saintstack · 2023-04-26T20:02:17Z

fdbserver/KeyValueStoreShardedRocksDB.actor.cpp

+				fOptions.wait = true;
+				fOptions.allow_write_stall = true;
+
+				for (auto shard : (*a.dirtyShards)) {


Will this be expensive? Will it up the latency on commits being done inline w/ the commit?

I would expect latency to go up. As we do commit every 5 sec, it shouldn't affect other parts of the system. I added a knob to control blocking/non-blocking flush. We could do non-blocking flush if latency is an issue.

fdb-windows-ci · 2023-04-29T01:16:44Z

Doxense CI Report for Windows 10

Commit ID: d895149
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2023-04-29T01:20:57Z

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: d895149
Duration 0:25:53
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-29T01:29:35Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: d895149
Duration 0:34:29
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-29T01:56:08Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: d895149
Duration 1:01:02
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-04-29T02:15:29Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: d895149
Duration 1:20:19
Result: ❌ FAILED
Error: Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

Co-authored-by: Jingyu Zhou <[email protected]>

fdb-windows-ci · 2023-05-02T19:31:00Z

Doxense CI Report for Windows 10

Commit ID: 267e167
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2023-05-02T19:43:06Z

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: 05e410f
Duration 0:27:16
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-02T19:51:35Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 267e167
Duration 0:35:34
Result: ❌ FAILED
Error: Error while executing command: ctest -j ${NPROC} --no-compress-output -T test --output-on-failure. Reason: exit status 8
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-02T19:52:47Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: 05e410f
Duration 0:36:56
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-02T20:19:24Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 05e410f
Duration 1:03:38
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-02T20:19:32Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 05e410f
Duration 1:03:44
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-02T20:19:59Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 267e167
Duration 1:03:57
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-02T20:42:43Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 267e167
Duration 1:26:40
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2023-05-02T20:42:52Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 05e410f
Duration 1:27:01
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

liquid-helium · 2023-05-02T21:47:54Z

fdbserver/KeyValueStoreShardedRocksDB.actor.cpp

+				}
 			}

-			a.done.send(Void());


What's the reason to move this line?

We're blocking future commits. We don't want to accept more commits before the flush finished.

saintstack · 2023-05-02T21:42:59Z

fdbclient/ServerKnobs.cpp

 	init( ROCKSDB_MAX_OPEN_FILES,                              50000 ); // Should be smaller than OS's fd limit.
+	init( ROCKSDB_USE_POINT_DELETE_FOR_SYSTEM_KEYS,            false ); if (isSimulated) ROCKSDB_USE_POINT_DELETE_FOR_SYSTEM_KEYS = deterministicRandom()->coinflip();
+	init( ROCKSDB_CF_RANGE_DELETION_LIMIT,                      1000 );
+	init (ROCKSDB_WAIT_ON_CF_FLUSH,                             true ); if (isSimulated) ROCKSDB_WAIT_ON_CF_FLUSH = deterministicRandom()->coinflip();


Is this a change in behavior? We wait on flush now where before we didn't?

Previously flushes are scheduled by rocksdb and running in background. In this case, we're blocking future write. It makes sense to wait for flushes. However the storage server could observe a higher commit latency due to this. If the high latency causes too much noise, we can turn it off.

foundationdb-ci · 2023-05-03T06:21:37Z

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: 267e167
Duration 0:25:16
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-03T06:31:52Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: 267e167
Duration 0:35:34
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-03T06:50:04Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 267e167
Duration 0:53:45
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-03T06:52:54Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 267e167
Duration 0:56:34
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2023-05-03T07:16:51Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 267e167
Duration 1:20:29
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

* Cherrypick #9665 * Cherrypick #9746 * Log background error and add knobs for memory tuning. (#9841) * error logger * recovery mode * Range deletion memory usage improvements (#10048) * Fix build error. * initialize counter * Log physical shard size in KVS * fix build error

Yao Xiao added 3 commits April 25, 2023 15:29

Reproduce OOM

ac3ee68

Flush after clear range

f54e813

Verify range deletions after commit

b02503c

xy-54321 force-pushed the convert-clear branch from 6b7902b to b02503c Compare April 25, 2023 22:30

comment

31d80a9

xy-54321 force-pushed the convert-clear branch from 467349c to 31d80a9 Compare April 25, 2023 22:43

saintstack reviewed Apr 26, 2023

View reviewed changes

fix simulation errors

d895149

xy-54321 requested review from jzhou77, liquid-helium, neethuhaneesha and saintstack April 29, 2023 01:02

Update fdbserver/KeyValueStoreShardedRocksDB.actor.cpp

267e167

Co-authored-by: Jingyu Zhou <[email protected]>

jzhou77 approved these changes May 2, 2023

View reviewed changes

liquid-helium reviewed May 2, 2023

View reviewed changes

saintstack approved these changes May 2, 2023

View reviewed changes

xy-54321 closed this May 3, 2023

xy-54321 reopened this May 3, 2023

xy-54321 merged commit e9d0ab9 into apple:release-7.2 May 3, 2023

xy-54321 deleted the convert-clear branch May 3, 2023 18:27

xy-54321 added a commit to xy-54321/foundationdb that referenced this pull request May 4, 2023

Range deletion memory usage improvements (apple#10048)

6b2dcab

xy-54321 added a commit to xy-54321/foundationdb that referenced this pull request May 9, 2023

Range deletion memory usage improvements (apple#10048)

0bb9f33

xy-54321 added a commit to xy-54321/foundationdb that referenced this pull request May 10, 2023

Range deletion memory usage improvements (apple#10048)

7a07116

xy-54321 added a commit to xy-54321/foundationdb that referenced this pull request May 10, 2023

Range deletion memory usage improvements (apple#10048)

2d1b5d0

xy-54321 added a commit to xy-54321/foundationdb that referenced this pull request May 10, 2023

Range deletion memory usage improvements (apple#10048)

a67cfe1

xy-54321 added a commit to xy-54321/foundationdb that referenced this pull request May 10, 2023

Range deletion memory usage improvements (apple#10048)

df12da5

xy-54321 added a commit to xy-54321/foundationdb that referenced this pull request May 11, 2023

Range deletion memory usage improvements (apple#10048)

8c62a82

Range deletion memory usage improvements #10048

Range deletion memory usage improvements #10048

Uh oh!

Conversation

xy-54321 commented Apr 25, 2023

Code-Reviewer Section

For Release-Branches

Uh oh!

fdb-windows-ci commented Apr 25, 2023

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Apr 25, 2023

Result of foundationdb-pr-clang on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 25, 2023

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 25, 2023

Result of foundationdb-pr-clang on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 25, 2023

Result of foundationdb-pr-clang on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 26, 2023

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 26, 2023

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 26, 2023

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 26, 2023

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 26, 2023

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 26, 2023

Result of foundationdb-pr-clang on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 26, 2023

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Apr 26, 2023

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

saintstack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdb-windows-ci commented Apr 29, 2023

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Apr 29, 2023

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Uh oh!

foundationdb-ci commented Apr 29, 2023

Result of foundationdb-pr-macos on macOS Ventura 13.x

Uh oh!

foundationdb-ci commented Apr 29, 2023

Result of foundationdb-pr-clang on Linux CentOS 7

Uh oh!