mapleFU commented on code in PR #2332:
URL: https://github.com/apache/kvrocks/pull/2332#discussion_r1673338874
##########
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;
Review Comment:
You can denote should it be nullptr
##########
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 {
Review Comment:
Anyway `Context` is so important, so maybe some comment should be better
here, including:
1. API
2. ThreadSafety
3. Limitations
##########
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
##########
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();
Review Comment:
const method 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]
