git-hulk commented on code in PR #2332:
URL: https://github.com/apache/kvrocks/pull/2332#discussion_r1715170232
##########
src/storage/storage.h:
##########
@@ -361,8 +368,78 @@ class Storage {
rocksdb::WriteOptions default_write_opts_ = rocksdb::WriteOptions();
- rocksdb::Status writeToDB(const rocksdb::WriteOptions &options,
rocksdb::WriteBatch *updates);
+ rocksdb::Status writeToDB(engine::Context &ctx, const rocksdb::WriteOptions
&options, rocksdb::WriteBatch *updates);
void recordKeyspaceStat(const rocksdb::ColumnFamilyHandle *column_family,
const rocksdb::Status &s);
};
+/// Context passes fixed snapshot and batche between APIs
Review Comment:
```suggestion
/// Context passes fixed snapshot and batch between APIs
```
##########
src/storage/storage.h:
##########
@@ -225,32 +227,37 @@ class Storage {
Status ApplyWriteBatch(const rocksdb::WriteOptions &options, std::string
&&raw_batch);
rocksdb::SequenceNumber LatestSeqNumber();
- [[nodiscard]] rocksdb::Status Get(const rocksdb::ReadOptions &options, const
rocksdb::Slice &key, std::string *value);
- [[nodiscard]] rocksdb::Status Get(const rocksdb::ReadOptions &options,
rocksdb::ColumnFamilyHandle *column_family,
+ [[nodiscard]] rocksdb::Status Get(engine::Context &ctx, const
rocksdb::ReadOptions &options,
Review Comment:
Would it be better to provide an API group to allow us to omit the
options(get from the context)? I see many places are using the options from the
context.
##########
src/storage/storage.cc:
##########
@@ -1224,4 +1261,31 @@ bool Storage::ReplDataManager::FileExists(Storage
*storage, const std::string &d
return crc == tmp_crc;
}
+[[nodiscard]] rocksdb::ReadOptions Context::GetReadOptions() const {
+ rocksdb::ReadOptions read_options;
+ if (is_txn_mode) read_options.snapshot = snapshot;
+ return read_options;
+}
+
+[[nodiscard]] rocksdb::ReadOptions Context::DefaultScanOptions() const {
+ rocksdb::ReadOptions read_options = storage->DefaultScanOptions();
+ if (is_txn_mode) read_options.snapshot = snapshot;
+ return read_options;
+}
+
+[[nodiscard]] rocksdb::ReadOptions Context::DefaultMultiGetOptions() const {
+ rocksdb::ReadOptions read_options = storage->DefaultMultiGetOptions();
+ if (is_txn_mode) read_options.snapshot = snapshot;
+ return read_options;
+}
+
+void Context::ResetLatestSnapshot() {
Review Comment:
It should be `RefreshLatestSnapshot`? since it will assign a new one after
releasing the old one.
##########
src/storage/storage.cc:
##########
@@ -1224,4 +1261,31 @@ bool Storage::ReplDataManager::FileExists(Storage
*storage, const std::string &d
return crc == tmp_crc;
}
+[[nodiscard]] rocksdb::ReadOptions Context::GetReadOptions() const {
+ rocksdb::ReadOptions read_options;
+ if (is_txn_mode) read_options.snapshot = snapshot;
+ return read_options;
+}
+
+[[nodiscard]] rocksdb::ReadOptions Context::DefaultScanOptions() const {
+ rocksdb::ReadOptions read_options = storage->DefaultScanOptions();
+ if (is_txn_mode) read_options.snapshot = snapshot;
+ return read_options;
+}
+
+[[nodiscard]] rocksdb::ReadOptions Context::DefaultMultiGetOptions() const {
+ rocksdb::ReadOptions read_options = storage->DefaultMultiGetOptions();
+ if (is_txn_mode) read_options.snapshot = snapshot;
+ return read_options;
+}
+
+void Context::ResetLatestSnapshot() {
+ auto guard = storage->WriteLockGuard();
+ if (snapshot) {
+ storage->GetDB()->ReleaseSnapshot(snapshot);
+ }
+ snapshot = storage->GetDB()->GetSnapshot();
+ batch->Clear();
Review Comment:
Need to check if the batch is null?
##########
src/storage/storage.h:
##########
@@ -361,8 +368,78 @@ class Storage {
rocksdb::WriteOptions default_write_opts_ = rocksdb::WriteOptions();
- rocksdb::Status writeToDB(const rocksdb::WriteOptions &options,
rocksdb::WriteBatch *updates);
+ rocksdb::Status writeToDB(engine::Context &ctx, const rocksdb::WriteOptions
&options, rocksdb::WriteBatch *updates);
void recordKeyspaceStat(const rocksdb::ColumnFamilyHandle *column_family,
const rocksdb::Status &s);
};
+/// Context passes fixed snapshot and batche between APIs
+///
+/// Limitations: Performing a large number of writes on the same Context may
reduce performance.
+/// Please choose to use the same Context or create a new Context based on the
actual situation.
+///
+/// Context does not provide thread safety guarantees and is generally only
passed as a parameter between APIs.
+struct Context {
+ engine::Storage *storage = nullptr;
+ /// If is_txn_mode is true, snapshot should be specified instead of nullptr
when used,
+ /// and should be consistent with snapshot in ReadOptions to avoid ambiguity.
+ /// Normally it will be fixed to the latest Snapshot when the Context is
constructed.
+ /// If is_txn_mode is false, the snapshot is nullptr.
+ const rocksdb::Snapshot *snapshot = nullptr;
+ std::unique_ptr<rocksdb::WriteBatchWithIndex> batch = nullptr;
+
+ /// is_txn_mode is used to determine whether the current Context is in
transactional mode,
+ /// if it is not transactional mode, then Context is equivalent to a Storage
+ bool is_txn_mode = true;
+
+ /// NoTransactionContext returns a Context with a is_txn_mode of false
+ static Context NoTransactionContext(engine::Storage *storage) { return
Context(storage, false); }
+
+ /// GetReadOptions returns a default ReadOptions, and if is_txn_mode = true,
then its snapshot is specified by the
+ /// Context
+ [[nodiscard]] rocksdb::ReadOptions GetReadOptions() const;
+ /// DefaultScanOptions returns a DefaultScanOptions, and if is_txn_mode =
true, then its snapshot is specified by the
+ /// Context. Otherwise it is the same as Storage::DefaultScanOptions
+ [[nodiscard]] rocksdb::ReadOptions DefaultScanOptions() const;
+ /// DefaultMultiGetOptions returns a DefaultMultiGetOptions, and if
is_txn_mode = true, then its snapshot is specified
+ /// by the Context. Otherwise it is the same as
Storage::DefaultMultiGetOptions
+ [[nodiscard]] rocksdb::ReadOptions DefaultMultiGetOptions() const;
+
+ void ResetLatestSnapshot();
+
+ /// TODO: Change it to defer getting the context, and the snapshot is pinned
after the first read operation
+ explicit Context(engine::Storage *storage) : storage(storage) {
+ auto guard = storage->ReadLockGuard();
+ snapshot = storage->GetDB()->GetSnapshot(); // NOLINT
+ }
+ ~Context() {
+ if (storage) {
+ auto guard = storage->WriteLockGuard();
+ if (storage->GetDB()) {
Review Comment:
Should check if the snapshot is null here?
##########
src/search/hnsw_indexer.h:
##########
@@ -41,16 +41,17 @@ struct HnswNode {
HnswNode(NodeKey key, uint16_t level);
- StatusOr<HnswNodeFieldMetadata> DecodeMetadata(const SearchKey& search_key,
engine::Storage* storage) const;
+ StatusOr<HnswNodeFieldMetadata> DecodeMetadata(engine::Context& ctx, const
SearchKey& search_key,
Review Comment:
We can remove the `storage` parameter here?
##########
src/storage/storage.cc:
##########
@@ -587,15 +588,20 @@ Status Storage::GetWALIter(rocksdb::SequenceNumber seq,
std::unique_ptr<rocksdb:
rocksdb::SequenceNumber Storage::LatestSeqNumber() { return
db_->GetLatestSequenceNumber(); }
-rocksdb::Status Storage::Get(const rocksdb::ReadOptions &options, const
rocksdb::Slice &key, std::string *value) {
- return Get(options, db_->DefaultColumnFamily(), key, value);
+rocksdb::Status Storage::Get(engine::Context &ctx, const rocksdb::ReadOptions
&options, const rocksdb::Slice &key,
+ std::string *value) {
+ return Get(ctx, options, db_->DefaultColumnFamily(), key, value);
}
-rocksdb::Status Storage::Get(const rocksdb::ReadOptions &options,
rocksdb::ColumnFamilyHandle *column_family,
- const rocksdb::Slice &key, std::string *value) {
+rocksdb::Status Storage::Get(engine::Context &ctx, const rocksdb::ReadOptions
&options,
+ rocksdb::ColumnFamilyHandle *column_family, const
rocksdb::Slice &key,
+ std::string *value) {
+ DCHECK_EQ(ctx.snapshot->GetSequenceNumber(),
options.snapshot->GetSequenceNumber());
rocksdb::Status s;
if (is_txn_mode_ && txn_write_batch_->GetWriteBatch()->Count() > 0) {
s = txn_write_batch_->GetFromBatchAndDB(db_.get(), options, column_family,
key, value);
+ } else if (ctx.batch) {
Review Comment:
Do we need to check if `is_txn_mode` is true here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
