PokIsemaine commented on code in PR #2332:
URL: https://github.com/apache/kvrocks/pull/2332#discussion_r1673450957
##########
src/storage/storage.cc:
##########
@@ -604,16 +610,19 @@ rocksdb::Status Storage::Get(const rocksdb::ReadOptions
&options, rocksdb::Colum
return s;
}
-rocksdb::Status Storage::Get(const rocksdb::ReadOptions &options, const
rocksdb::Slice &key,
+rocksdb::Status Storage::Get(engine::Context &ctx, const rocksdb::ReadOptions
&options, const rocksdb::Slice &key,
rocksdb::PinnableSlice *value) {
- return Get(options, db_->DefaultColumnFamily(), key, 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, rocksdb::PinnableSlice
*value) {
+rocksdb::Status Storage::Get(engine::Context &ctx, const rocksdb::ReadOptions
&options,
+ rocksdb::ColumnFamilyHandle *column_family, const
rocksdb::Slice &key,
+ rocksdb::PinnableSlice *value) {
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) {
+ s = ctx.batch->GetFromBatchAndDB(db_.get(), options, column_family, key,
value);
} else {
s = db_->Get(options, column_family, key, value);
}
Review Comment:
Recently, I reconsidered merging `Context` and `ReadOptions` and made some
small attempts to encapsulate the complete `ReadOptions` into Context while
removing the ReadOptions parameter from the API. I discovered another point
that needs attention: in fact, the context we need to propagate between
multiple API calls only includes `Batch` and `Snapshot`. For other settings of
`ReadOptions`, they should not be propagated as context.
For example: API X uses `DefaultScanOptions` to set `ReadOptions`, and at
the same time, it calls API Y, while API Y only uses the default `ReadOptions`
(except for `Snapshot`). One common situation is that API Y directly uses the
settings of `DefaultScanOptions` because for default `ReadOptions`, we might
habitually use the existing `ReadOptions` in ctx and mistakenly assume it to be
the default `ReadOptions`. Actually, we need to reset `ReadOptions` for each
API and use the Snapshot in Context.
If not merged, it can explicitly isolate the `ReadOptions` of each API,
forcing the creation of new `ReadOptions`.
What are your thoughts and suggestions? @PragmaTwice
--
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]
