-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Ingest sst files rather than their keyvalue content. #12079
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
Changes from all commits
55687a1
b64d216
8c700f5
138d142
c411dca
4fb4764
549e205
ac0307f
647c689
1756790
7f648d5
5e8ca3f
4921ce0
7eb4094
ad0ed1a
51b7d34
915bb7d
a782b5b
a70af03
982f6b3
825e824
ade5ae5
1ad37f7
596c432
2e43909
c285b92
a117e10
494c25d
46368f1
bb7901f
3770148
ddd9099
c910bfc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,8 @@ class IKeyValueStore : public IClosable { | |
// Returns true if the KV store supports shards, i.e., implements addRange(), removeRange(), and | ||
// persistRangeMapping(). | ||
virtual bool shardAware() const { return false; } | ||
// Returns true if the store supports external SST file ingestion. | ||
virtual bool supportsSstIngestion() const { return false; } | ||
virtual void set(KeyValueRef keyValue, const Arena* arena = nullptr) = 0; | ||
virtual void clear(KeyRangeRef range, const Arena* arena = nullptr) = 0; | ||
virtual Future<Void> canCommit() { return Void(); } | ||
|
@@ -134,6 +136,9 @@ class IKeyValueStore : public IClosable { | |
// Delete a checkpoint. | ||
virtual Future<Void> deleteCheckpoint(const CheckpointMetaData& checkpoint) { throw not_implemented(); } | ||
|
||
// Compact a range of keys in the store | ||
virtual Future<Void> compactRange(KeyRangeRef range) { throw not_implemented(); } | ||
|
||
/* | ||
Concurrency contract | ||
Causal consistency: | ||
|
@@ -157,6 +162,13 @@ class IKeyValueStore : public IClosable { | |
// Obtain the encryption mode of the storage. The encryption mode needs to match the encryption mode of the cluster. | ||
virtual Future<EncryptionAtRestMode> encryptionMode() = 0; | ||
|
||
// the files in localFileSets. | ||
// Throws an error if the store does not support SST ingestion or if ingestion fails. | ||
// It is the responsibility of the caller to ensure the directory exists and the fileSetMap is valid. | ||
virtual Future<Void> ingestSSTFiles(std::shared_ptr<BulkLoadFileSetKeyMap> localFileSets) { | ||
throw not_implemented(); | ||
} | ||
|
||
protected: | ||
virtual ~IKeyValueStore() {} | ||
}; | ||
|
@@ -167,18 +179,44 @@ ACTOR static Future<Void> replaceRange_impl(IKeyValueStore* self, | |
state int sinceYield = 0; | ||
state const KeyValueRef* kvItr = data.begin(); | ||
state KeyRangeRef rangeRef = range; | ||
if (rangeRef.empty()) { | ||
state bool errorOccurred = false; | ||
|
||
try { | ||
if (rangeRef.empty()) { | ||
return Void(); | ||
} | ||
|
||
// Clear the range first | ||
self->clear(rangeRef); | ||
|
||
// Set the new data | ||
for (; kvItr != data.end(); kvItr++) { | ||
try { | ||
self->set(*kvItr); | ||
if (++sinceYield > 1000) { | ||
wait(yield()); | ||
sinceYield = 0; | ||
} | ||
} catch (Error& e) { | ||
errorOccurred = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we want to change this? Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only real change here is the addition of a wait on commit (later) before we progress. The rest is trying to get better logging when error. In the ctest runs there was crashing doing replace range (centos7, gcc, only). I added commit to emulate what we do over in ingest where we wait on version change after clear... . Was thinking this crash like what we were seeing before waiting on version change after clear over on ingest... It looks to have helped. I can try removing... after the centos7/gcc build stabilizes.
|
||
TraceEvent(SevError, "ReplaceRangeError") | ||
.error(e) | ||
.detail("Key", kvItr->key) | ||
.detail("ValueSize", kvItr->value.size()); | ||
throw; | ||
} | ||
} | ||
|
||
// Wait for the changes to be durable | ||
wait(self->commit()); | ||
return Void(); | ||
} | ||
self->clear(rangeRef); | ||
for (; kvItr != data.end(); kvItr++) { | ||
self->set(*kvItr); | ||
if (++sinceYield > 1000) { | ||
wait(yield()); | ||
sinceYield = 0; | ||
} catch (Error& e) { | ||
if (e.code() == error_code_actor_cancelled) { | ||
throw; | ||
} | ||
TraceEvent(SevError, "ReplaceRangeFailed").error(e).detail("Range", rangeRef).detail("DataSize", data.size()); | ||
throw; | ||
} | ||
return Void(); | ||
} | ||
|
||
#include "flow/unactorcompiler.h" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,7 +58,7 @@ function start_fdb_cluster { | |
"${local_build_dir}" \ | ||
--knobs "${knobs}" \ | ||
--stateless_count 1 --replication_count 1 --logs_count 1 \ | ||
--storage_count "${ss_count}" --storage_type ssd \ | ||
--storage_count "${ss_count}" --storage_type ssd-rocksdb-v1 \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to randomly choose between sqlite and rocksdb? for testing coverage There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dunno. rocksdb is the standard, not sqllite. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want bulkload feature to stably work for both sqlite and rocksdb There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me make a new PR for this that allows setting of storage engine to use in ctest. |
||
--dump_pids on \ | ||
> >(tee "${output}") \ | ||
2> >(tee "${output}" >&2) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do
if (isSimulated) BULK_LOAD_USE_SST_INGEST = deterministicRandom()->coinflip()
to improve the test coverage?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done