PokIsemaine commented on code in PR #2332:
URL: https://github.com/apache/kvrocks/pull/2332#discussion_r1715348947
##########
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:
Do you mean to put ReadOptions in Context?
Here are the ideas I mentioned before
> 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.
Do you have any suggestions?
--
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]
