mapleFU commented on code in PR #2332:
URL: https://github.com/apache/kvrocks/pull/2332#discussion_r1685663110
##########
src/storage/storage.h:
##########
@@ -358,8 +365,49 @@ 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;
+ /// 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
+ const rocksdb::Snapshot *snapshot = nullptr;
+ std::unique_ptr<rocksdb::WriteBatchWithIndex> batch = nullptr;
+
+ /// GetReadOptions returns a ReadOptions whose snapshot is specified by
Context
+ [[nodiscard]] rocksdb::ReadOptions GetReadOptions() const;
Review Comment:
This api is good, still `Storage` supports some api, like:
```c++
rocksdb::ReadOptions DefaultScanOptions() const;
rocksdb::ReadOptions DefaultMultiGetOptions() const;
```
We may wraps the same api for the Context
##########
src/storage/storage.h:
##########
@@ -358,8 +365,49 @@ 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.
Review Comment:
LGTM except the style
```suggestion
/// 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.
```
##########
src/cluster/slot_migrate.cc:
##########
@@ -1235,7 +1238,9 @@ Status SlotMigrator::sendSnapshotByRawKV() {
read_options.snapshot = slot_snapshot_;
rocksdb::Slice prefix_slice(prefix);
read_options.iterate_lower_bound = &prefix_slice;
- engine::DBIterator iter(storage_, read_options);
+ // TODO: ctx
+ engine::Context ctx(storage_);
+ engine::DBIterator iter(ctx, storage_, read_options);
Review Comment:
It's a bit weird taht read_options and iter might have different snapshot 🤔
Would we change to:
```
engine::Context ctx(storage_);
rocksdb::ReadOptions read_options = ctx->DefaultScanOptions();
```
So that we can use same scan-options here
##########
src/storage/storage.h:
##########
@@ -358,8 +365,43 @@ 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);
};
+struct Context {
+ engine::Storage *storage = nullptr;
+ const rocksdb::Snapshot *snapshot = nullptr;
+ std::unique_ptr<rocksdb::WriteBatchWithIndex> batch = nullptr;
+
+ rocksdb::ReadOptions GetReadOptions();
+ const rocksdb::Snapshot *GetSnapShot();
+ void SetLatestSnapshot();
+
+ explicit Context(engine::Storage *storage) : storage(storage),
snapshot(storage->GetDB()->GetSnapshot()) {}
Review Comment:
> We can add a todo for changing it to lazily get the context
Would you mind adding a todo here?
##########
src/cluster/slot_migrate.cc:
##########
@@ -1235,7 +1238,9 @@ Status SlotMigrator::sendSnapshotByRawKV() {
read_options.snapshot = slot_snapshot_;
rocksdb::Slice prefix_slice(prefix);
read_options.iterate_lower_bound = &prefix_slice;
- engine::DBIterator iter(storage_, read_options);
+ // TODO: ctx
Review Comment:
why a todo here?
##########
src/cluster/slot_migrate.cc:
##########
@@ -1259,7 +1264,7 @@ Status SlotMigrator::sendSnapshotByRawKV() {
GET_OR_RET(batch_sender.Put(storage_->GetCFHandle(ColumnFamilyID::Metadata),
iter.Key(), iter.Value()));
- auto subkey_iter = iter.GetSubKeyIterator();
+ auto subkey_iter = iter.GetSubKeyIterator(ctx);
Review Comment:
`iter` contains a iterator, what's the considering to re-add a ctx here?
##########
src/storage/storage.h:
##########
@@ -358,8 +365,49 @@ 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;
+ /// 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
+ const rocksdb::Snapshot *snapshot = nullptr;
+ std::unique_ptr<rocksdb::WriteBatchWithIndex> batch = nullptr;
+
+ /// GetReadOptions returns a ReadOptions whose snapshot is specified by
Context
+ [[nodiscard]] rocksdb::ReadOptions GetReadOptions() const;
+ void SetLatestSnapshot();
Review Comment:
The interface is good but could we naming it `ResetLatestSnapshot` ( since
it release and get a new snapshot)
##########
src/search/indexer.cc:
##########
@@ -275,17 +281,20 @@ Status
IndexUpdater::UpdateHnswVectorIndex(std::string_view key, const kqir::Val
auto storage = indexer->storage;
auto hnsw = HnswIndex(search_key, vector, storage);
+ // TODO: ctx?
Review Comment:
What's the meaning of this todo?
--
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]
