Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Improve the RTC process of Read/Write model #2629

Merged
merged 7 commits into from
Jul 12, 2024

Conversation

chenbt-hz
Copy link
Collaborator

@chenbt-hz chenbt-hz commented Apr 28, 2024

about: #2542
改动:在读请求放入队列前,先进行判断是否在cache中读取

Summary by CodeRabbit

  • New Features

    • Introduced batch reading of commands from cache to improve performance.
  • Bug Fixes

    • Enhanced Redis client setup in integration tests to ensure stability.
  • Chores

    • Added a manual trigger for workflows in the GitHub Actions configuration.

@github-actions github-actions bot added ✏️ Feature New feature or request 🧹 Updates This will not be worked on labels Apr 28, 2024
@chejinge
Copy link
Collaborator

我有个疑问,如果这个命令应该在cache中读取 但是命不中,还是得放入队列吧,只有命中了才不用放入队列
判断命中不命中就相当于查询了一次cache了

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


I have a question. If this command should be read from the cache but misses, it still has to be put into the queue. Only if it hits, there is no need to put it into the queue.
Determining whether the hit is a hit or not is equivalent to querying the cache once.

@AlexStocks AlexStocks requested a review from cheniujh May 31, 2024 12:50
src/pika_command.cc Outdated Show resolved Hide resolved
src/pika_client_conn.cc Outdated Show resolved Hide resolved
@github-actions github-actions bot added the 📒 Documentation Improvements or additions to documentation label Jun 3, 2024
@AlexStocks
Copy link
Collaborator

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

4, because the PR involves significant changes across multiple files including core functionality and command behavior modifications. The changes impact the command execution flow, particularly in how commands are processed for cache reads and the conditions under which commands are queued. Understanding and verifying these changes requires a deep understanding of the existing architecture and the implications of the new flags and command behaviors.

🧪 Relevant tests

No

⚡ Possible issues

Thread Safety: The BatchReadCmdInCache function modifies shared state such as resp_num and resp_array without apparent synchronization mechanisms in a multi-threaded context, which might lead to race conditions.

Error Handling: In the BatchReadCmdInCache function, if AuthRequired() returns true and the command does not pass the authentication check, it sets an error response but does not clean up or reset the state, potentially leading to incorrect behavior or leaks in subsequent operations.

🔒 Security concerns

No

Code feedback:
relevant filesrc/pika_client_conn.cc
suggestion      

Consider adding thread safety mechanisms, such as mutexes, to protect the shared state modifications in the BatchReadCmdInCache function. This is important to prevent data races and ensure the integrity of shared data across multiple threads. [important]

relevant lineresp_num.store(static_cast(argvs.size()));

relevant filesrc/pika_client_conn.cc
suggestion      

Implement proper cleanup and state reset in the BatchReadCmdInCache function when an authentication error occurs. This could prevent potential memory leaks or undefined behaviors in subsequent operations. [important]

relevant linec_ptr->res().SetRes(CmdRes::kErrOther, "NOAUTH Authentication required.");

relevant filesrc/pika_client_conn.cc
suggestion      

Optimize the command lookup in BatchReadCmdInCache by reducing redundant lookups for the same command, which could improve performance by minimizing map access operations. [medium]

relevant linestd::shared_ptr c_ptr = g_pika_cmd_table_manager->GetCmd(argv[0]);

relevant filesrc/pika_command.cc
suggestion      

Refactor the DoReadCommandInCache method to separate concerns more clearly, possibly by splitting the method into smaller, more focused methods. This can improve readability and maintainability of the code. [medium]

relevant linebool Cmd::DoReadCommandInCache(const HintKeys& hint_keys) {

@AlexStocks
Copy link
Collaborator

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Ensure command processing only continues if the batch read operation was successful

Consider checking the return value of BatchReadCmdInCache before proceeding with
scheduling the client pool. This ensures that the command processing only continues if the
batch read operation was successful.

src/pika_client_conn.cc [282-288]

 if (BatchReadCmdInCache(argvs)){
+  g_pika_server->ScheduleClientPool(&DoBackgroundTask, arg, is_slow_cmd);
   return;
 }
-g_pika_server->ScheduleClientPool(&DoBackgroundTask, arg, is_slow_cmd);
 
Suggestion importance[1-10]: 9

Why: This suggestion ensures that the command processing only continues if the batch read operation was successful, which is crucial for the correct functioning of the code. It addresses a potential issue where the client pool might be scheduled even if the batch read operation failed.

9
Thread safety
Ensure thread safety and correct handling by using fetch_sub for atomic decrement

Replace the manual decrement of resp_num with a more robust method that ensures thread
safety and correct handling under various conditions.

src/pika_client_conn.cc [354-355]

 *resp_ptr = std::move(c_ptr->res().message());
-resp_num--;
+resp_num.fetch_sub(1, std::memory_order_relaxed);
 
Suggestion importance[1-10]: 8

Why: This suggestion enhances thread safety by using fetch_sub for atomic decrement of resp_num. It ensures correct handling under various conditions, which is important for concurrent environments.

8
Performance
Improve memory allocation efficiency by reserving capacity for resp_array outside the loop

Avoid using std::make_sharedstd::string() repeatedly inside a loop. Instead, reserve the necessary
capacity for resp_array outside the loop to improve memory allocation efficiency.

src/pika_client_conn.cc [327-329]

+resp_array.reserve(argvs.size());
 for (const auto& argv : argvs) {
   std::shared_ptr<std::string> resp_ptr = std::make_shared<std::string>();
   resp_array.push_back(resp_ptr);
   ...
 }
 
Suggestion importance[1-10]: 7

Why: This suggestion improves memory allocation efficiency by reserving capacity for resp_array outside the loop. It is a minor optimization but can enhance performance, especially with large input sizes.

7
Maintainability
Improve code readability and maintainability by separating locking logic into a dedicated method

Refactor the DoReadCommandInCache method to separate concerns of locking and cache
reading. This improves readability and maintainability.

src/pika_command.cc [920-927]

-if (!IsSuspend()) {
-  db_->DBLockShared();
+ManageLock();
+...
+void Cmd::ManageLock() {
+  if (!IsSuspend()) {
+    db_->DBLockShared();
+    DEFER {
+      db_->DBUnlockShared();
+    };
+  }
 }
-DEFER {
-  if (!IsSuspend()) {
-    db_->DBUnlockShared();
-  }
-};
-...
 
Suggestion importance[1-10]: 6

Why: This suggestion improves code readability and maintainability by separating the locking logic into a dedicated method. While it does not change the functionality, it makes the code easier to understand and maintain.

6

@@ -272,6 +274,17 @@ void PikaClientConn::ProcessRedisCmds(const std::vector<net::RedisCmdArgsType>&
std::string opt = argvs[0][0];
pstd::StringToLower(opt);
bool is_slow_cmd = g_pika_conf->is_slow_cmd(opt);
std::shared_ptr<Cmd> c_ptr = g_pika_cmd_table_manager->GetCmd(opt);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一个优先级比较低的优化点:这个拷贝的c_ptr能否通过异步任务的参数传递到threadPoolWorker让其复用,这样就不用在一次指向链路上从cmd_table上copy两次命令了

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这块拷贝猜测大约会损耗多少性能?
感觉没有合适的地方传呢?如果要改,有初步思路吗?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果要传递,或许可以通过arg传递到线程池。但确实要看一下是否真的对性能有损耗 ,真的有损耗再考虑把。这个可以最后再考虑。

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

想到了优化的办法:
鉴于现在模式反了过来,只是拦截少数的请求来原地查询cache,可以先以字符串的形式将这些需要拦截的命令放在一个unordered_se中,然后每次我们在这个位置直接用opt去查询set即可,就避免了额外clone一次命令对象的行为。另外,由于我们这个unordered_set是只读的(拦截哪些请求不支持动态修改,只是最开始初始化的时候才会写这个set),我们并发查询这个set时,连锁都不用去加。

cheniujh
cheniujh previously approved these changes Jun 14, 2024
resp_num.store(static_cast<int32_t>(argvs.size()));
bool read_status = true;

for (const auto& argv : argvs) {
Copy link
Collaborator

@cheniujh cheniujh Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果有一整个batch进来(pipeline),可能会有些命令命中Cache,有些不命中,这种case可能也需要考虑一下。
另外如果是一个整个batch进来,尤其是pipeline的batch,可以考虑要不要干脆直接放行走线程池? 因为pipeline写一般多一些,而且如果pipeline单个batch命令量很大,当前这个networker也可能会在这耽误一会?

如果让batch直接放行走常规处理路径,其实不但大大降低了实现复杂度,而且一样能满足绝大部分场景的需求(绝大部分场景还是没有用pipeline的,都是一个网络请求中只有一条命令)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

对于batch和pipeline,在isCacheRead时把会MUTIL开头的请求过滤,这样就还是走常规处理。

Copy link
Collaborator

@cheniujh cheniujh Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi只是告知一个事务的开始,而且multi(事务)包含的一批命令会先被缓存在pika内部(应该是其他地方),而不是一次性发过来的,可能光靠multi判断不了是否是batch哈,batch应该是只有pipeline会有。

我建议这里可以显式地去判断命令是否有多条,有多条就直接放行,这样还有一个好处,就是后面有人再读代码的话,一眼就能看出batch是放行的。

Copy link

coderabbitai bot commented Jun 19, 2024

Walkthrough

The changes introduce a new method BatchReadCmdInCache to the PikaClientConn class, enabling batch reading of commands from the cache. Several Redis commands are now flagged for pre-queue reading. Minor adjustments were made to workflow triggers and test setups, ensuring better management and reliability of Redis commands and tests.

Changes

Files/Groups Summary
.github/workflows/pika.yml Added a workflow_dispatch trigger alongside existing branch triggers.
include/pika_client_conn.h Added BatchReadCmdInCache method and before_queue_ts_ variable to the PikaClientConn class.
include/pika_command.h Added kCmdReadBeforeQueue flag and new methods isCacheRead() and DoReadCommandInCache() to Cmd class.
src/pika_client_conn.cc Implemented BatchReadCmdInCache method in PikaClientConn class.
src/pika_command.cc Updated flags for various Cmd objects including kCmdReadBeforeQueue, kCmdFlagsSlow, and kCmdFlagsFast.
tests/integration/stream_test.go Modified client setup logic in tests, specifically handling GlobalBefore conditionally and removing AfterEach.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant PikaClientConn
    participant Cache
    participant CommandProcessor

    Client->>PikaClientConn: Send batch commands
    PikaClientConn->>Cache: Check cache for commands
    alt Commands found in cache
        Cache-->>PikaClientConn: Return cached commands
        PikaClientConn->>CommandProcessor: Process cached commands
        CommandProcessor->>PikaClientConn: Return processed data
        PikaClientConn-->>Client: Send response
    else Commands not found in cache
        PikaClientConn->>CommandProcessor: Process fresh commands
        CommandProcessor->>Cache: Cache the commands
        CommandProcessor->>PikaClientConn: Return processed data
        PikaClientConn-->>Client: Send response
    end
Loading

Poem

In the cache, commands now reside,
With PikaClientConn, they smoothly glide,
New flags flutter, pre-queue beams,
Enhanced by methods, like in dreams.
The tests are sharper, workflows bright,
Redis dances through the night.


Tip

Early access features: enabled

We are currently testing the following features in early access:

  • OpenAI gpt-4o model for code reviews and chat: OpenAI claims that this model is better at understanding and generating code than the previous models. We seek your feedback over the next few weeks before making it generally available.

Note:

  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.
  • Please join our Discord Community to provide feedback and report issues.
  • OSS projects are currently opted into early access features by default.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e7edec6 and 1685e3e.

Files selected for processing (6)
  • .github/workflows/pika.yml (1 hunks)
  • include/pika_client_conn.h (1 hunks)
  • include/pika_command.h (3 hunks)
  • src/pika_client_conn.cc (3 hunks)
  • src/pika_command.cc (7 hunks)
  • tests/integration/stream_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • .github/workflows/pika.yml
Additional comments not posted (9)
src/pika_client_conn.cc (1)

21-21: Ensure newly included file is necessary and utilized.

Verification successful

The newly included file scope_record_lock.h is indeed utilized in src/pika_client_conn.cc, as evidenced by the reference to pstd::lock::MultiRecordLock.

  • src/pika_client_conn.cc: pstd::lock::MultiRecordLock record_lock(c_ptr->db_->LockMgr());
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the newly included file is utilized in the code.

# Test: Search for usage of `scope_record_lock.h`. Expect: At least one usage.
rg --type cpp 'scope_record_lock' src/pika_client_conn.cc

Length of output: 105



Script:

#!/bin/bash
# Description: Identify entities defined in `scope_record_lock.h`.

# Locate the file first
scope_record_lock_file=$(fd 'scope_record_lock.h')

# Extract defined entities (classes, structs, functions, etc.)
rg --type cpp --pcre2 '(class|struct|enum|typedef|using|namespace|template)\s+\w+' "$scope_record_lock_file"

Length of output: 398



Script:

#!/bin/bash
# Description: Identify entities defined in `scope_record_lock.h`.

# Locate the file first
fd 'scope_record_lock.h' | while read -r scope_record_lock_file; do
  echo "Entities in $scope_record_lock_file:"
  rg --type cpp --pcre2 '(class|struct|enum|typedef|using|namespace|template)\s+\w+' "$scope_record_lock_file"
done

Length of output: 986



Script:

#!/bin/bash
# Description: Verify the utilization of entities from `scope_record_lock.h` in `src/pika_client_conn.cc`.

# Search for references to `ScopeRecordLock`, `MultiScopeRecordLock`, and `MultiRecordLock`.
rg --type cpp 'ScopeRecordLock|MultiScopeRecordLock|MultiRecordLock' src/pika_client_conn.cc

Length of output: 162

include/pika_command.h (3)

293-293: Added a new command flag kCmdReadBeforeQueue to improve read/write process by checking cache before queuing.

This addition aligns with the PR's objective to optimize command handling by potentially reducing queue operations for cache hits. Ensure that this new flag is integrated correctly in all relevant command implementations.


540-540: Added method isCacheRead() to check if a command should be read from cache.

This method likely serves as a helper to determine whether the new kCmdReadBeforeQueue behavior should be applied. It's crucial to ensure that this method is used consistently across the codebase where applicable.


584-584: Added method DoReadCommandInCache() which likely implements the actual cache reading logic.

This method is central to the PR's functionality, handling the reading of commands from cache. It's important to review its implementation in the corresponding .cc file to ensure it handles all edge cases, especially those concerning thread safety and error handling as highlighted by previous reviews.

tests/integration/stream_test.go (1)

127-129: Proper use of conditional execution in BeforeEach to ensure safety.

src/pika_command.cc (4)

246-246: Ensure the new flag kCmdReadBeforeQueue is correctly documented and integrated.

The addition of kCmdReadBeforeQueue to the GetCmd seems aligned with the PR's objective to handle cache reads before queuing. Please ensure that the integration of this new flag is well documented in the codebase to maintain clarity for future maintenance.


395-395: Review the implementation and consistency of kCmdReadBeforeQueue across hash commands.

The addition of the kCmdReadBeforeQueue flag to hash-related commands (HGetCmd, HGetallCmd, HExistsCmd, HMgetCmd) is consistent with the PR's objectives. It's crucial to ensure that these changes are reflected in the system's behavior as expected, particularly in terms of cache interaction and performance.

Also applies to: 399-399, 403-403, 423-423


763-763: Cross-verify the integration of transaction and ACL related commands with the system.

The changes to transaction and ACL related commands are crucial and should be thoroughly tested to ensure they do not introduce any regressions or unintended behaviors, especially in multi-threaded scenarios where race conditions could occur.

Also applies to: 769-769, 773-773, 776-776, 779-779, 782-782


739-739: Ensure proper testing and documentation for Pub/Sub commands with the new flag adjustments.

The modifications to Pub/Sub commands (PublishCmd, SubscribeCmd, UnSubscribeCmd, PSubscribeCmd, PUnSubscribeCmd, PubSubCmd) with new flags should be accompanied by adequate testing and documentation to ensure that they perform as expected in a production environment.

Also applies to: 743-743, 747-747, 751-751, 755-755, 759-759

Comment on lines +123 to +124
client = redis.NewClient(PikaOption(SINGLEADDR))
client.FlushDB(ctx)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Ensure proper error handling for FlushDB in stream_test.go.

  • tests/integration/stream_test.go (lines 123-124)
Analysis chain

Ensure proper error handling for FlushDB.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify proper error handling for `FlushDB` in the test setup.

# Test: Search for error handling patterns in the test setup. Expect: Proper error handling.
rg --type go $'client.FlushDB.*\\.Err\\(\\).*NotTo\\(HaveOccurred\\(\\)\\)'

Length of output: 1156



Script:

#!/bin/bash
# Description: Verify proper error handling for `FlushDB` in stream_test.go at lines 123-124.

# Test: Search for error handling patterns in stream_test.go. Expect: Proper error handling.
rg --type go --context 5 'client.FlushDB' tests/integration/stream_test.go

Length of output: 343

@@ -69,6 +69,7 @@ class PikaClientConn : public net::RedisConn {

void ProcessRedisCmds(const std::vector<net::RedisCmdArgsType>& argvs, bool async, std::string* response) override;

bool BatchReadCmdInCache(const std::vector<net::RedisCmdArgsType>& argvs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add documentation for BatchReadCmdInCache.

The newly added method BatchReadCmdInCache lacks documentation. Consider adding a comment explaining what the method does, its parameters, and its return value. This will enhance readability and maintainability.

Comment on lines 277 to 286
std::shared_ptr<Cmd> c_ptr = g_pika_cmd_table_manager->GetCmd(opt);

if (PIKA_CACHE_NONE != g_pika_conf->cache_mode()){
if ( c_ptr && c_ptr->isCacheRead() ){
// read in cache
if (BatchReadCmdInCache(argvs)){
return;
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize command lookup to prevent redundant cache checks.

The current implementation checks for cache read capability and then performs a cache read within a conditional statement that checks for cache mode. This could lead to redundant lookups for the same command when the cache mode is not PIKA_CACHE_NONE. Consider refactoring to ensure the command is only looked up once.

- if (PIKA_CACHE_NONE != g_pika_conf->cache_mode()){
-   if (c_ptr && c_ptr->isCacheRead()){
+ if (PIKA_CACHE_NONE != g_pika_conf->cache_mode() && c_ptr && c_ptr->isCacheRead()){
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std::shared_ptr<Cmd> c_ptr = g_pika_cmd_table_manager->GetCmd(opt);
if (PIKA_CACHE_NONE != g_pika_conf->cache_mode()){
if ( c_ptr && c_ptr->isCacheRead() ){
// read in cache
if (BatchReadCmdInCache(argvs)){
return;
}
}
}
std::shared_ptr<Cmd> c_ptr = g_pika_cmd_table_manager->GetCmd(opt);
if (PIKA_CACHE_NONE != g_pika_conf->cache_mode() && c_ptr && c_ptr->isCacheRead()){
// read in cache
if (BatchReadCmdInCache(argvs)){
return;
}
}

Comment on lines 323 to 367
bool PikaClientConn::BatchReadCmdInCache(const std::vector<net::RedisCmdArgsType>& argvs){
resp_num.store(static_cast<int32_t>(argvs.size()));
bool read_status = true;

for (const auto& argv : argvs) {
std::shared_ptr<std::string> resp_ptr = std::make_shared<std::string>();
resp_array.push_back(resp_ptr);
std::shared_ptr<Cmd> c_ptr = g_pika_cmd_table_manager->GetCmd(argv[0]);
if (!c_ptr) {
return false;
}
// Check authed
if (AuthRequired()) { // the user is not authed, need to do auth
if (!(c_ptr->flag() & kCmdFlagsNoAuth)) {
c_ptr->res().SetRes(CmdRes::kErrOther, "NOAUTH Authentication required.");
return false;
}
}
// Initial
c_ptr->Initial(argv, current_db_);
pstd::lock::MultiRecordLock record_lock(c_ptr->db_->LockMgr());
auto cur_keys = c_ptr->current_key();
if (!cur_keys.empty()){
record_lock.Lock(cur_keys);
}

read_status = c_ptr->DoReadCommandInCache();
time_stat_->process_done_ts_ = pstd::NowMicros();
auto cmdstat_map = g_pika_cmd_table_manager->GetCommandStatMap();
(*cmdstat_map)[argv[0]].cmd_count.fetch_add(1);
(*cmdstat_map)[argv[0]].cmd_time_consuming.fetch_add(time_stat_->total_time());
*resp_ptr = std::move(c_ptr->res().message());
resp_num--;
record_lock.Unlock(cur_keys);
}

if(!read_status){
return read_status;
}else{
TryWriteResp();
return read_status;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve error handling and thread safety in BatchReadCmdInCache.

The method BatchReadCmdInCache processes multiple commands and adjusts resp_num directly. This could lead to race conditions in a multi-threaded environment. Consider using atomic operations or mutexes to protect this shared state. Additionally, there's no error handling if a command fails to initialize or authenticate, which could leave the system in an inconsistent state.

+ std::lock_guard<std::mutex> lock(resp_mutex); // Protect resp_num with a mutex
  for (const auto& argv : argvs) {
    std::shared_ptr<std::string> resp_ptr = std::make_shared<std::string>();
    resp_array.push_back(resp_ptr);
    std::shared_ptr<Cmd> c_ptr = g_pika_cmd_table_manager->GetCmd(argv[0]);
    if (!c_ptr) {
-     return false;
+     read_status = false;
+     break; // Exit loop on failure but ensure cleanup
    }
    ...
  }
+ if (!read_status) {
+   // Perform necessary cleanup or state reset here
+ }

Committable suggestion was skipped due to low confidence.

@@ -282,7 +282,7 @@ void InitCmdTable(CmdTable* cmd_table) {
cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameAppend, std::move(appendptr)));
////MgetCmd
std::unique_ptr<Cmd> mgetptr =
std::make_unique<MgetCmd>(kCmdNameMget, -2, kCmdFlagsRead | kCmdFlagsKv | kCmdFlagsDoThroughDB | kCmdFlagsUpdateCache | kCmdFlagsReadCache | kCmdFlagsFast);
std::make_unique<MgetCmd>(kCmdNameMget, -2, kCmdFlagsRead | kCmdFlagsKv | kCmdFlagsDoThroughDB | kCmdFlagsUpdateCache | kCmdFlagsReadCache | kCmdReadBeforeQueue | kCmdFlagsFast);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The executed shell scripts provided some useful insights:

  1. Usage of kCmdReadBeforeQueue with MgetCmd:

    • Found in src/pika_command.cc.
  2. References to MgetCmd in the main codebase:

    • Multiple references found in src/pika_command.cc, src/pika_kv.cc, and src/pika_hash.cc.
  3. Test cases related to MgetCmd:

    • No direct matches found in the ./tests/ directory.
  4. Indirect references or related tests:

    • No indirect references or related tests found.

Based on these results, it appears that while the kCmdReadBeforeQueue flag is properly integrated with MgetCmd in the main codebase, there are no direct or indirect tests for MgetCmd in the test files. This suggests that the new flag's integration is not being tested, which is crucial for ensuring its functionality and performance impact.

  • Action Required: Add test cases for MgetCmd with the kCmdReadBeforeQueue flag to ensure proper testing of this integration.
Analysis chain

Validate the integration and testing of the kCmdReadBeforeQueue flag for MgetCmd.

The addition of the kCmdReadBeforeQueue flag to MgetCmd should be thoroughly tested, especially since this command can significantly impact performance due to its nature of handling multiple keys.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Ensure that `MgetCmd` with the new `kCmdReadBeforeQueue` flag is properly integrated and tested.

# Test: Search for integration tests involving `MgetCmd`.
rg --type go "MgetCmd" ./tests/integration/

Length of output: 43



Script:

#!/bin/bash
# Description: Ensure that `MgetCmd` with the new `kCmdReadBeforeQueue` flag is properly integrated and tested.

# Step 1: Verify the usage of `kCmdReadBeforeQueue` flag with `MgetCmd`.
echo "Searching for usage of kCmdReadBeforeQueue with MgetCmd..."
rg "kCmdReadBeforeQueue" | rg "MgetCmd"

# Step 2: Check for any test cases related to `MgetCmd` across the codebase.
echo "Searching for test cases related to MgetCmd..."
rg "MgetCmd" ./tests/

# Step 3: Ensure that `MgetCmd` with the new flag is properly integrated.
echo "Checking for references to MgetCmd in the main codebase..."
rg "MgetCmd" ./src/

# Step 4: Validate if there are any indirect references or tests for MgetCmd that might not be captured by the initial search.
echo "Searching for indirect references or related tests..."
rg "MgetCmd" ./tests/ | rg -v "integration"

Length of output: 2192

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1685e3e and b9e4891.

Files selected for processing (6)
  • .github/workflows/pika.yml (1 hunks)
  • include/pika_client_conn.h (1 hunks)
  • include/pika_command.h (3 hunks)
  • src/pika_client_conn.cc (3 hunks)
  • src/pika_command.cc (7 hunks)
  • tests/integration/stream_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
  • .github/workflows/pika.yml
  • include/pika_client_conn.h
  • include/pika_command.h
  • src/pika_client_conn.cc
  • src/pika_command.cc
  • tests/integration/stream_test.go

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b9e4891 and 6e462fc.

Files selected for processing (2)
  • src/pika_client_conn.cc (3 hunks)
  • src/pika_command.cc (7 hunks)
Files skipped from review as they are similar to previous changes (2)
  • src/pika_client_conn.cc
  • src/pika_command.cc

@chenbt-hz
Copy link
Collaborator Author

@chejinge @cheniujh 两位大佬,当前PR只添加了get、mget、hget、hmget、hgetall,有时间帮看下其他哪些命令是有必要补充的?

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


@chejinge @cheniujh Two big guys, the current PR only adds get, mget, hget, hmget, hgetall. If you have time, can you help me find out which other commands need to be added?

@@ -282,7 +282,7 @@ void InitCmdTable(CmdTable* cmd_table) {
cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameAppend, std::move(appendptr)));
////MgetCmd
std::unique_ptr<Cmd> mgetptr =
std::make_unique<MgetCmd>(kCmdNameMget, -2, kCmdFlagsRead | kCmdFlagsKv | kCmdFlagsDoThroughDB | kCmdFlagsUpdateCache | kCmdFlagsReadCache | kCmdFlagsFast);
std::make_unique<MgetCmd>(kCmdNameMget, -2, kCmdFlagsRead | kCmdFlagsKv | kCmdFlagsDoThroughDB | kCmdFlagsUpdateCache | kCmdFlagsReadCache | kCmdReadBeforeQueue | kCmdFlagsFast);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这种复杂命令最好不要这么做,因为你在缓存中没有没有办法标记,很有可能一部分在缓存中,一部分在DB中,这样命中也不是,没命中也不算的

src/pika_client_conn.cc Outdated Show resolved Hide resolved
@chejinge
Copy link
Collaborator

凡是需要走DB的命令建议直接放行,要不会有拖慢网络线程,影响性能的风险

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


All commands that need to go to the DB are recommended to be released directly, otherwise there will be no risk of slowing down the network thread and affecting performance.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6e462fc and ea811d3.

Files selected for processing (2)
  • .github/workflows/pika.yml (1 hunks)
  • src/pika_command.cc (7 hunks)
Files skipped from review as they are similar to previous changes (2)
  • .github/workflows/pika.yml
  • src/pika_command.cc

@chenbt-hz
Copy link
Collaborator Author

凡是需要走DB的命令建议直接放行,要不会有拖慢网络线程,影响性能的风险

已沟通,当前需要移除mget等命令的支持

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


It is recommended that all commands that need to go to the DB be released directly, otherwise there will be no risk of slowing down the network thread and affecting performance.

Communicated, currently support for mget and other commands needs to be removed

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ea811d3 and 6b81628.

Files selected for processing (6)
  • .github/workflows/pika.yml (1 hunks)
  • include/pika_client_conn.h (3 hunks)
  • include/pika_command.h (4 hunks)
  • src/pika_client_conn.cc (5 hunks)
  • src/pika_command.cc (6 hunks)
  • tests/integration/stream_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
  • .github/workflows/pika.yml
  • include/pika_client_conn.h
  • include/pika_command.h
  • src/pika_client_conn.cc
  • src/pika_command.cc
  • tests/integration/stream_test.go

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6b81628 and 04e9306.

Files selected for processing (1)
  • src/pika_client_conn.cc (5 hunks)
Files skipped from review as they are similar to previous changes (1)
  • src/pika_client_conn.cc

@chejinge chejinge changed the base branch from unstable to RTC July 2, 2024 02:34
@AlexStocks AlexStocks merged commit b05398a into OpenAtomFoundation:RTC Jul 12, 2024
13 of 14 checks passed
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Jul 30, 2024
…#2629)

* (Demo) Do read cmd before task queue. && add workflow_dispatch for manual action

* Check authed and write lock
,fix go test error in MacOS and cache mode judge

* fix some ut error by  commands filter  and return logic

* rollback some flag,but add kCmdReadBeforeQueuefor get mget hget hget hgetall,hmget

* fix mem error in macos

* move mget and hmget;add before_queue_time metrics

* fix cost to copy cmd_table by remove c_ptr
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Jul 31, 2024
…#2629)

* (Demo) Do read cmd before task queue. && add workflow_dispatch for manual action

* Check authed and write lock
,fix go test error in MacOS and cache mode judge

* fix some ut error by  commands filter  and return logic

* rollback some flag,but add kCmdReadBeforeQueuefor get mget hget hget hgetall,hmget

* fix mem error in macos

* move mget and hmget;add before_queue_time metrics

* fix cost to copy cmd_table by remove c_ptr
AlexStocks pushed a commit that referenced this pull request Jul 31, 2024
* feat: Improve the RTC process of Read/Write model  (#2629)

* (Demo) Do read cmd before task queue. && add workflow_dispatch for manual action

* Check authed and write lock, fix go test error in MacOS and cache mode judge

* fix some ut error by  commands filter  and return logic

* rollback some flag,but add kCmdReadBeforeQueuefor get mget hget hget hgetall,hmget

* move mget and hmget;add before_queue_time metrics

* fix cost to copy cmd_table by remove c_ptr

---------

Co-authored-by: chenbt <34958405+chenbt-hz@users.noreply.github.com>
cheniujh added a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
…omFoundation#2837)

* feat: Improve the RTC process of Read/Write model  (OpenAtomFoundation#2629)

* (Demo) Do read cmd before task queue. && add workflow_dispatch for manual action

* Check authed and write lock, fix go test error in MacOS and cache mode judge

* fix some ut error by  commands filter  and return logic

* rollback some flag,but add kCmdReadBeforeQueuefor get mget hget hget hgetall,hmget

* move mget and hmget;add before_queue_time metrics

* fix cost to copy cmd_table by remove c_ptr

---------

Co-authored-by: chenbt <34958405+chenbt-hz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.5.5 4.0.1 📒 Documentation Improvements or additions to documentation ✏️ Feature New feature or request 🧹 Updates This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants