refactor: Improve consistency and isolation semantics by adding Context parameter to DB API

[PokIsemaine]

issue: #2310
This is just a draft PR to demonstrate ideas, so there are a lot of imperfections.

A. How:

Add a Context parameter to each DB API. The Context contains the fixed snapshot during the entire call process and the resulting operations (stored using rocksdb::WriteBatchWithIndex).
Start of call: Construct Context, use snapshot for fixed call, and pass it to DB API as a parameter

During the call:

Read operation:

• Use rocksdb::WriteBatchWithIndex's GetFromBatchAndDB to read data. Only two parts of data can be seen in one call: the write operation data in this call stored in rocksdb::WriteBatchWithIndex; the snapshot is Context.snapshot DB data at the time. (NewIterator and MultiGet use the same method)
• You will not see data from other concurrent API calls.

Write operations:

• Kvrocks current write operation: GetBatchBase gets a WriteBatch, then adds operations to WriteBatch, and then writes the operations in WriteBatch to DB.
• Modify the writeToDB part of Write and implement a WriteBatch::Handler (tentatively named WriteBatchIndexer) for iterating the operations in WriteBatch and appending them to WriteBatchWithIndex of Context.

B. Possible codes to pay attention to:

storage:

• Structure of Context
• writeToDB and Get
• batch_indexer.h: WriteBatchIndexer

redis_db: As an example, other APIs are derived from the redis_db API

cmd_xx: Construct a new Context when executing

C. Description: This section is used to explain the modifications that you may feel strange.

CI part: I temporarily added the -v parameter to observe the running of golang integration tests in GitHub Action in more detail

Test part:

• C++ unit test: I added engine::Context to TestBase to simplify test initialization. During the test process, I used SetLatestSnapshot of Context to get the latest snapshot and clear the batch in Context. This is because some tests A large number of loops are used. If the same ctx is used all the time, the data of all loops will be accumulated on it, and lead to extremely long time consuming. This is unnecessary because for the test case we do not require multiple APIs to be guaranteed to read the same snapshot.
• Go integration test: I added Sleep to the BlockCommand test of list and zset, because I found that sometimes the execution order of multiple client requests would be inconsistent with the order of code writing due to fluctuations, resulting in infinite waiting or errors. As a result, for reference to other parts of the code and for convenience, I added some sleep to ensure the order.

TODO: TODO: ctx in the implementation code means that I am not sure whether the ctx setting here is correct. Should it come from the upper parameters or create a Context myself? Where is the boundary of the Context. I'm not familiar with the search, slot, etc. parts yet.

D. Improvement: The following are the points that I currently consider need to be improved.

• Context should be able to replace the original LatestSnapshot, GetOptions, and merge part of the API's ReadOptions
• Improve comments and code
• Correctness verification:
  • Unit test for WriteBatch::Handler
  • jepsen, sync point, and script tools verify the isolation level (expected isolation level of snapshot) and possible concurrency exceptions
    Performance testing, especially testing of DeleteRange related operations (if not ideal, try disabling it)

E. suggestion:

• Because it is a draft PR, it may be more efficient to discuss rationality first and then discuss optimization details.
• When discussing, you can use A-E to indicate which part is being discussed

[PokIsemaine]

I'll be moving forward with this PR these days, and since it's been a long time, I'll start by resolving the current conflicts and synchronizing the code with the latest unstable.

[PokIsemaine]

NOTE:
This is a relatively large PR that involves most of the functional modules. I tried to split them into smaller PRs but failed because the changes are interrelated and propagate among each other.

Therefore, to better advance this work, I believe we need to conduct the review gradually and multiple times. At the start of each review, I will post a "REVIEW X" comment to inform which parts are being reviewed and highlight any points that need attention or discussion. When this review is complete, please reply to the comment with "REVIEW X ok" and I will start the next review. If you have any questions or suggestions during this period, please include the prefix "REVIEW X" to indicate which stage it belongs to.

Although there are still some TODO items, I plan to discuss and refine them during the review process as you become more familiar with the background of these TODOs.

If you think this approach is feasible, I will soon start the first review, change the PR from Draft to Review status, and then switch it back to Draft status for modifications.
The overall process is: REVIEW 1 => DRAFT modification => REVIEW 1 ok => REVIEW 2 => DRAFT modification => REVIEW 2 ok....

[PokIsemaine]

REVIEW 1:
Notes: The main goal of REVIEW 1 is to help everyone get familiar with the changes that have been made. These modifications will serve as a template for other parts, so I will select the core, stable, and simple parts to review. Before the review, please browse the previous explanations, comments, and related issues(#2310).

• Most APIs have been added with the engine::Context parameter. Please note the APIs that do not have this parameter added, as they may have been overlooked.
• Many APIs originally had the ReadOptions parameter. Adding another Context parameter might overlap with ReadOptions in functionality, so consider whether these two parameters should be merged. For example, it might be necessary to use both the batch of Context and the complete ReadOptions settings (not just Snapshot). In order to understand everyone's thoughts, I have not modified it directly. If you think it should be merged, please provide suggestions.
• Some APIs use LatestSnapshot, and it's necessary to consider whether this is mandatory. If not, consider using context directly to get the specified snapshot. I haven't modified it directly in order to understand what everyone thinks. If you think it should be replaced with Context, please provide suggestions.
  Files to review:

storage/storage.h
storage/storage.cc
storage/redis_db.h
storage/redis_db.cc

commands/cmd_string.cc
types/redis_string.h
types/redis_string.cc

cppunit/test_base.h
cppunit/types/string_test.cc

Currently, we haven't assigned reviewers for this PR. If it's convenient, I would like to kindly request @mapleFU , @git-hulk , and @PragmaTwice to assist with the review. I understand this may take some time, so please feel free to do it at your convenience. Your feedback and suggestions will be greatly appreciated. Thank you very much for your help.

[PragmaTwice]

Thank you very much for your contribution!

This PR is quite large, so we may need a longer time to gradually familiarize ourselves with the changes and provide our suggestions : )

[PragmaTwice]

Some APIs use LatestSnapshot, and it's necessary to consider whether this is mandatory. If not, consider using context directly to get the specified snapshot. I haven't modified it directly in order to understand what everyone thinks. If you think it should be replaced with Context, please provide suggestions.

It can be removed if the Context class handles them well.

[mapleFU]

Great work! I'll take a carefully pass tonight!

[PokIsemaine]

Currently, the main focus is on syncing with the unstable branch.

If it's convenient, I suggest starting the review and discussion as soon as possible, because the PR will become more complex as we progress.

For less critical improvements and optimizations, we can temporarily mark them as TODOs to be addressed in future iterations. I believe we should prioritize completing a basic functional version first.

Thank you again for taking the time to provide guidance and suggestions. 😽

[mapleFU]

Thanks for the efforts:

Go integration test: I added Sleep to the BlockCommand test of list and zset, because I found that sometimes the execution order of multiple client requests would be inconsistent with the order of code writing due to fluctuations, resulting in infinite waiting or errors. As a result, for reference to other parts of the code and for convenience, I added some sleep to ensure the order.

Actually I didn't fully get what does this part means, can you explain it with some examples?

[PokIsemaine]

Thanks for the efforts:

Go integration test: I added Sleep to the BlockCommand test of list and zset, because I found that sometimes the execution order of multiple client requests would be inconsistent with the order of code writing due to fluctuations, resulting in infinite waiting or errors. As a result, for reference to other parts of the code and for convenience, I added some sleep to ensure the order.

Actually I didn't fully get what does this part means, can you explain it with some examples?

This was a previous judgment, but there was a misunderstanding. Now I think these issues are also caused by #2473

[mapleFU]

LGTM % some nits! Thanks!

[mapleFU]

@PragmaTwice @git-hulk This pr is ready % some nits and comments, and it's ready for review, would you mind take a look?

[git-hulk]

@PragmaTwice @git-hulk This pr is ready % some nits and comments, and it's ready for review, would you mind take a look?

Sure, will retake another pass tomorrow.

[git-hulk]

@PokIsemaine Sorry for the late review, a few comments inline.

[mapleFU]

Unrelated to this pr: I think we can also add a server side conf to allow disable is_txn_mode, this is useful when we have some performance issue with DeleteRange or make something wrong. We can merge this first and add this in a separate patch

[PokIsemaine]

Unrelated to this pr: I think we can also add a server side conf to allow disable is_txn_mode, this is useful when we have some performance issue with DeleteRange or make something wrong. We can merge this first and add this in a separate patch

I agree, this PR has a wider impact and we should provide the option to turn it on and off.

In subsequent observations, in addition to performance and errors, memory usage also needs attention, because we have an additional WriteBatchWithIndex.

[mapleFU]

+1, but I think we need fast collect all of us' approve since it's really huge
@PragmaTwice @git-hulk

[mapleFU]

I'll draft to merge this patch if no negative comment this week

[mapleFU]

Will merge this after ci pass

[git-hulk]

Will merge this after ci pass

I'm good with this.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud