fix: query result cache v2 #3895
Conversation
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (1)
You can disable this status message by setting the WalkthroughThe updates primarily involve modifications to caching functionalities, including restructuring the Changes
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/common/meta/search.rs (1 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (3 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (5 hunks)
- src/service/search/cluster/cacher.rs (2 hunks)
Additional context used
Path-based instructions (5)
src/common/meta/search.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/grpc/request/query_cache.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cluster/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cache/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/search/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (7)
src/proto/proto/cluster/querycache.proto (1)
34-37: Field renumbering is accurate.The field numbers have been renumbered correctly without altering the content.
src/common/meta/search.rs (1)
Line range hint
14-14: Verify the removal ofhas_pre_cache_delta.Ensure that no logic in the codebase depends on the removed
has_pre_cache_deltafield.src/handler/grpc/request/query_cache.rs (1)
19-19: Verify the removal ofdeltasandhas_pre_cache_delta.Ensure that no logic in the codebase depends on the removed
deltasandhas_pre_cache_deltafields.src/service/search/cluster/cacher.rs (1)
175-175: Direct assignment of an empty vector todeltasis correct.The change simplifies the logic and ensures that
deltasis always an empty vector.src/service/search/cache/cacher.rs (2)
268-268: Direct assignment of an empty vector todeltasis correct.The change simplifies the logic and ensures that
deltasis always an empty vector.
Line range hint
126-147: Verify the removal ofdeltasandhas_pre_cache_delta.Ensure that no logic in the codebase depends on the removed
deltasandhas_pre_cache_deltafields.src/handler/http/request/search/mod.rs (1)
332-341: Verify the correctness of the logging conditions and statements.The added logging block checks for
result_cache_enabled,print_key_sql, andhas_cached_databefore logging query start and end times. Ensure these conditions are correct and the logged values are accurate.Verification successful
Logging conditions and statements are correct.
The conditions
result_cache_enabled,print_key_sql, andhas_cached_dataare consistently used throughout the code to control caching behavior and logging. The logged values are accurately referenced from the request object.
- The conditions ensure that logging occurs only when caching is enabled, key SQL printing is enabled, and cached data is available.
- The logged values (
req.query.start_timeandreq.query.end_time) are correctly referenced.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the logging conditions and statements. # Test: Search for the usage of `result_cache_enabled`, `print_key_sql`, and `has_cached_data`. # Expect: Only occurances of the new logging statements. rg --type rust 'result_cache_enabled|print_key_sql|has_cached_data'Length of output: 1370
Script:
#!/bin/bash # Description: Extract surrounding lines of the conditions to verify their correctness and context. # Extract 5 lines before and after each occurrence of the conditions in `mod.rs`. rg --type rust -A 5 -B 5 'result_cache_enabled|print_key_sql|has_cached_data' src/handler/http/request/search/mod.rsLength of output: 1969
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
from
3b002f5 to
2ccc463
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/common/meta/search.rs (1 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (3 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (5 hunks)
- src/service/search/cluster/cacher.rs (2 hunks)
Files skipped from review due to trivial changes (1)
- src/handler/http/request/search/mod.rs
Files skipped from review as they are similar to previous changes (3)
- src/common/meta/search.rs
- src/proto/proto/cluster/querycache.proto
- src/service/search/cluster/cacher.rs
Additional context used
Path-based instructions (2)
src/handler/grpc/request/query_cache.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cache/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (4)
src/handler/grpc/request/query_cache.rs (1)
19-19: Verify the removal ofdeltasandhas_pre_cache_deltafields.Ensure that the removal of these fields does not introduce breaking changes or affect the logic elsewhere in the codebase.
src/service/search/cache/cacher.rs (3)
268-268: Verify delta processing logic.Ensure that the changes in delta processing logic are correct and do not introduce any issues.
Verification successful
Verify delta processing logic.
The changes in delta processing logic are correct and do not introduce any issues.
- The function
calculate_deltas_v1correctly handles various conditions for query start and end times.- It appropriately updates the
deltasvector withQueryDeltastructures based on the differences between query and cache times.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of delta processing logic. # Test: Search for the usage of `calculate_deltas_v1` and verify the logic. Expect: Correct delta processing logic. rg --type rust 'calculate_deltas_v1'Length of output: 167
Script:
#!/bin/bash # Extract the implementation of `calculate_deltas_v1` from `src/service/search/cache/cacher.rs` ast-grep --lang rust --pattern $'fn calculate_deltas_v1($_) { $$$ }' src/service/search/cache/cacher.rs # Extract the usage of `calculate_deltas_v1` from `src/service/search/cache/cacher.rs` rg 'calculate_deltas_v1' src/service/search/cache/cacher.rsLength of output: 227
Script:
#!/bin/bash # Extract the implementation of `calculate_deltas_v1` by fetching lines around its definition rg -A 20 -B 2 'pub fn calculate_deltas_v1' src/service/search/cache/cacher.rs # Extract the usage context of `calculate_deltas_v1` by fetching lines around its usage rg -A 5 -B 5 'calculate_deltas_v1' src/service/search/cache/cacher.rsLength of output: 1527
Script:
#!/bin/bash # Extract additional lines to capture the complete implementation of `calculate_deltas_v1` rg -A 40 -B 2 'pub fn calculate_deltas_v1' src/service/search/cache/cacher.rsLength of output: 1747
Script:
#!/bin/bash # Extract additional lines to capture the complete implementation of `calculate_deltas_v1` rg -A 60 -B 2 'pub fn calculate_deltas_v1' src/service/search/cache/cacher.rsLength of output: 2320
243-257: Verify timestamp-based hit retention logic.Ensure that the changes in timestamp-based hit retention logic are correct and do not introduce any issues.
Verification successful
Verified timestamp-based hit retention logic.
The changes in timestamp-based hit retention logic are correctly implemented and do not introduce any issues.
- The logic correctly filters hits based on
start_time,end_time, anddiscard_ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of timestamp-based hit retention logic. # Test: Search for the usage of `get_cached_results` and verify the logic. Expect: Correct timestamp-based hit retention logic. rg --type rust 'get_cached_results'Length of output: 2066
Script:
#!/bin/bash # Extract the relevant code around the `get_cached_results` function in the identified files to verify the logic. # Extract 20 lines before and after the function definition to get sufficient context rg --type rust -A 20 -B 20 'pub async fn get_cached_results' src/service/search/cluster/cacher.rs src/service/search/cache/cacher.rsLength of output: 5945
Script:
#!/bin/bash # Extract the relevant code lines around the filtering logic within the `get_cached_results` function in `src/service/search/cache/cacher.rs` to verify the correctness. # Extract 20 lines before and after the filtering logic to get sufficient context rg --type rust -A 20 -B 20 'if (hit_ts <= end_time && hit_ts >= start_time) && hit_ts < discard_ts' src/service/search/cache/cacher.rsLength of output: 136
Script:
#!/bin/bash # Extract the entire `get_cached_results` function from `src/service/search/cache/cacher.rs` to ensure we capture all the relevant logic. # Extract the entire function definition to get sufficient context ast-grep --lang rust --pattern 'pub async fn get_cached_results($_) -> Option<CachedQueryResponse> { $$$ }' src/service/search/cache/cacher.rsLength of output: 144
Script:
#!/bin/bash # Extract the relevant code lines around the `for hit in &cached_response.hits` loop within the `get_cached_results` function in `src/service/search/cache/cacher.rs` to verify the correctness. # Extract 20 lines before and after the loop to get sufficient context rg --type rust -A 20 -B 20 'for hit in &cached_response.hits' src/service/search/cache/cacher.rsLength of output: 2195
Line range hint
126-147: Verify delta calculation logic.Ensure that the changes in delta calculation logic are correct and do not introduce any issues.
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
from
a3a8f93 to
22b4484
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
src/handler/grpc/request/query_cache.rs (2)
19-19: Remove unused import.The
QueryCacheResimport is not used in this file.- QueryCacheRequest, QueryCacheRes, QueryCacheResponse, QueryResponse, + QueryCacheRequest, QueryCacheResponse, QueryResponse,
Line range hint
28-36: Ensure proper error handling for JSON serialization.The
serde_json::to_veccall can fail and should be properly handled.- data: serde_json::to_vec(&res.cached_response).unwrap(), + data: match serde_json::to_vec(&res.cached_response) { + Ok(data) => data, + Err(e) => { + log::error!("Error serializing cached response: {:?}", e); + return Err(Status::internal("Internal server error")); + } + },src/service/search/cache/cacher.rs (1)
Line range hint
138-147: Simplify delta filtering logic.The filtering logic for
search_deltacan be simplified usingretain.- let search_delta: Vec<QueryDelta> = deltas - .iter() - .filter(|d| !d.delta_removed_hits) - .cloned() - .collect(); + deltas.retain(|d| !d.delta_removed_hits); + let search_delta = deltas;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/common/meta/search.rs (1 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (3 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (5 hunks)
- src/service/search/cluster/cacher.rs (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- src/common/meta/search.rs
- src/handler/http/request/search/mod.rs
- src/proto/proto/cluster/querycache.proto
- src/service/search/cluster/cacher.rs
Additional context used
Path-based instructions (2)
src/handler/grpc/request/query_cache.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cache/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cache/cacher.rs
Outdated
| matching_cache_meta.end_time | ||
| } | ||
| }; | ||
|
|
||
| if !remove_hits.is_empty() { | ||
| for delta in remove_hits { | ||
| for hit in &cached_response.hits { | ||
| let hit_ts = get_ts_value(result_ts_column, hit); | ||
|
|
||
| if !(hit_ts >= delta.delta_start_time | ||
| && hit_ts < delta.delta_end_time) | ||
| && (hit_ts <= end_time && hit_ts >= start_time) | ||
| && hit_ts < discard_ts | ||
| { | ||
| to_retain.push(hit.clone()); | ||
| } | ||
| } | ||
| for hit in &cached_response.hits { | ||
| let hit_ts = get_ts_value(result_ts_column, hit); | ||
|
|
||
| if (hit_ts <= end_time && hit_ts >= start_time) && hit_ts < discard_ts { | ||
| to_retain.push(hit.clone()); | ||
| } | ||
| cached_response.hits = to_retain; | ||
| cached_response.total = cached_response.hits.len(); | ||
| if discard_interval < 0 { | ||
| matching_cache_meta.end_time = discard_ts; | ||
| }; | ||
| } | ||
| cached_response.hits = to_retain; | ||
| cached_response.total = cached_response.hits.len(); |
There was a problem hiding this comment.
Optimize hit retention logic.
The hit retention logic can be optimized by using retain instead of creating a new vector.
- let mut to_retain = Vec::new();
- for hit in &cached_response.hits {
- let hit_ts = get_ts_value(result_ts_column, hit);
- if (hit_ts <= end_time && hit_ts >= start_time) && hit_ts < discard_ts {
- to_retain.push(hit.clone());
- }
- }
- cached_response.hits = to_retain;
+ cached_response.hits.retain(|hit| {
+ let hit_ts = get_ts_value(result_ts_column, hit);
+ (hit_ts <= end_time && hit_ts >= start_time) && hit_ts < discard_ts
+ });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.
| matching_cache_meta.end_time | |
| } | |
| }; | |
| if !remove_hits.is_empty() { | |
| for delta in remove_hits { | |
| for hit in &cached_response.hits { | |
| let hit_ts = get_ts_value(result_ts_column, hit); | |
| if !(hit_ts >= delta.delta_start_time | |
| && hit_ts < delta.delta_end_time) | |
| && (hit_ts <= end_time && hit_ts >= start_time) | |
| && hit_ts < discard_ts | |
| { | |
| to_retain.push(hit.clone()); | |
| } | |
| } | |
| for hit in &cached_response.hits { | |
| let hit_ts = get_ts_value(result_ts_column, hit); | |
| if (hit_ts <= end_time && hit_ts >= start_time) && hit_ts < discard_ts { | |
| to_retain.push(hit.clone()); | |
| } | |
| cached_response.hits = to_retain; | |
| cached_response.total = cached_response.hits.len(); | |
| if discard_interval < 0 { | |
| matching_cache_meta.end_time = discard_ts; | |
| }; | |
| } | |
| cached_response.hits = to_retain; | |
| cached_response.total = cached_response.hits.len(); | |
| matching_cache_meta.end_time | |
| } | |
| }; | |
| cached_response.hits.retain(|hit| { | |
| let hit_ts = get_ts_value(result_ts_column, hit); | |
| (hit_ts <= end_time && hit_ts >= start_time) && hit_ts < discard_ts | |
| }); | |
| cached_response.total = cached_response.hits.len(); |
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
from
22b4484 to
bfe232c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/common/meta/search.rs (1 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (3 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (5 hunks)
- src/service/search/cluster/cacher.rs (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/handler/grpc/request/query_cache.rs
- src/handler/http/request/search/mod.rs
- src/service/search/cache/cacher.rs
Additional context used
Path-based instructions (2)
src/common/meta/search.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cluster/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (3)
src/proto/proto/cluster/querycache.proto (1)
34-34: LGTM! Renumbering of fields is correct.The renumbering of fields in the
QueryCacheResmessage is correct and consistent with the rest of the file.src/common/meta/search.rs (1)
Line range hint
16-16:
LGTM! Removal ofhas_pre_cache_deltafield is correct.The removal of the
has_pre_cache_deltafield from theCachedQueryResponsestruct is consistent with the provided context and does not affect the rest of the struct definition.src/service/search/cluster/cacher.rs (1)
175-175: LGTM! Simplification ofdeltashandling is correct.The change to directly assign an empty vector to the
deltasfield is correct and simplifies the function.
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
from
3601047 to
3a56a51
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/common/meta/search.rs (1 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (3 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (5 hunks)
- src/service/search/cluster/cacher.rs (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/handler/grpc/request/query_cache.rs
- src/handler/http/request/search/mod.rs
- src/service/search/cache/cacher.rs
Additional context used
Path-based instructions (2)
src/common/meta/search.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cluster/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (3)
src/proto/proto/cluster/querycache.proto (1)
34-37: Field renumbering verification.The fields in the
QueryCacheResmessage have been renumbered. Ensure that these changes are reflected correctly in the corresponding codebase and documentation.Verification successful
Field renumbering verification complete.
The
QueryCacheResmessage insrc/proto/proto/cluster/querycache.protohas been renumbered correctly as follows:
has_cached_data = 2cache_query_response = 3cache_start_time = 4cache_end_time = 5These changes are reflected correctly in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the renumbering of fields in the QueryCacheRes message. # Test: Search for the usage of QueryCacheRes. Expect: Field numbers should match the new numbering. rg --type proto 'QueryCacheRes'Length of output: 61
Script:
#!/bin/bash # Description: Verify the renumbering of fields in the QueryCacheRes message. # Search for the definition of QueryCacheRes in .proto files. rg 'QueryCacheRes' --glob '*.proto' # Search for the usage of QueryCacheRes in the entire codebase. rg 'QueryCacheRes'Length of output: 1233
Script:
#!/bin/bash # Description: Verify the field numbers in the QueryCacheRes message definition and their usage. # Display the definition of QueryCacheRes in the .proto file. rg -A 10 'message QueryCacheRes' src/proto/proto/cluster/querycache.proto # Search for the usage of QueryCacheRes fields in the Rust file. rg 'QueryCacheRes' src/handler/grpc/request/query_cache.rsLength of output: 859
src/common/meta/search.rs (1)
Line range hint
14-14:
Field removal verification.The
has_pre_cache_deltafield has been removed from theCachedQueryResponsestruct. Ensure that these changes are reflected correctly in the corresponding codebase and documentation.src/service/search/cluster/cacher.rs (1)
175-175: Simplified delta handling.The
deltasfield is now directly assigned an empty vector. This simplifies the logic and removes unnecessary operations.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/handler/http/request/search/mod.rs (4 hunks)
- src/service/search/cache/cacher.rs (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/request/search/mod.rs
Additional context used
Path-based instructions (1)
src/service/search/cache/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (3)
src/service/search/cache/cacher.rs (3)
Line range hint
138-147:
LGTM! Delta handling logic is clear.The code changes are approved. The
search_deltacalculation correctly filters out deltas withdelta_removed_hitsset to true.
Line range hint
229-241:
Optimize hit retention logic.The hit retention logic can be optimized by using
retaininstead of creating a new vector.- let mut to_retain = Vec::new(); - for hit in &cached_response.hits { - let hit_ts = get_ts_value(result_ts_column, hit); - if (hit_ts <= end_time && hit_ts >= start_time) && hit_ts < discard_ts { - to_retain.push(hit.clone()); - } - } - cached_response.hits = to_retain; + cached_response.hits.retain(|hit| { + let hit_ts = get_ts_value(result_ts_column, hit); + (hit_ts <= end_time && hit_ts >= start_time) && hit_ts < discard_ts + });
263-263: Simplify delta handling logic.The delta handling logic has been simplified by directly assigning an empty vector, which is a valid simplification.
| let mut deltas = vec![]; | ||
| calculate_deltas_v1( | ||
| &ResultCacheMeta { | ||
| start_time: cached_resp.response_start_time, | ||
| end_time: cached_resp.response_end_time, | ||
| is_aggregate, | ||
| }, | ||
| req.query.start_time, | ||
| req.query.end_time, | ||
| &mut deltas, | ||
| ); |
There was a problem hiding this comment.
Remove redundant delta calculation.
The calculate_deltas_v1 function is used to calculate deltas but the result is not used in the subsequent code.
- let mut deltas = vec![];
- calculate_deltas_v1(
- &ResultCacheMeta {
- start_time: cached_resp.response_start_time,
- end_time: cached_resp.response_end_time,
- is_aggregate,
- },
- req.query.start_time,
- req.query.end_time,
- &mut deltas,
- );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.
| let mut deltas = vec![]; | |
| calculate_deltas_v1( | |
| &ResultCacheMeta { | |
| start_time: cached_resp.response_start_time, | |
| end_time: cached_resp.response_end_time, | |
| is_aggregate, | |
| }, | |
| req.query.start_time, | |
| req.query.end_time, | |
| &mut deltas, | |
| ); |
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
from
34425c3 to
0e1207a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/common/meta/search.rs (1 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (11 hunks)
- src/infra/src/cache/file_data/disk.rs (1 hunks)
- src/infra/src/cache/meta.rs (1 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (8 hunks)
- src/service/search/cluster/cacher.rs (7 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/common/meta/search.rs
- src/handler/grpc/request/query_cache.rs
- src/handler/http/request/search/mod.rs
- src/proto/proto/cluster/querycache.proto
- src/service/search/cache/cacher.rs
Additional context used
Path-based instructions (3)
src/infra/src/cache/meta.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cluster/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/infra/src/cache/file_data/disk.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (8)
src/infra/src/cache/meta.rs (1)
23-23: LGTM!The addition of the
is_descendingfield is consistent with the overall changes to support descending order in caching mechanisms.src/service/search/cluster/cacher.rs (6)
32-32: LGTM!The use of
CacheQueryRequestfor parameters is a good improvement for handling cache queries.
53-53: LGTM!The
ts_columnassignment is consistent with the overall changes.
67-79: LGTM!The creation of the
QueryCacheRequestobject with the new fields is consistent with the overall changes.
176-183: LGTM!The addition of the
is_descendingfield and the empty vector assignment fordeltasare consistent with the overall changes.
211-218: LGTM!The use of
CacheQueryRequestfor the local response is consistent with the overall changes.
230-235: LGTM!The logic for filtering and selecting the best cache result is clear and consistent with the changes.
src/infra/src/cache/file_data/disk.rs (1)
415-421: LGTM!The parsing and handling of the
is_descendingfield from the file metadata are consistent with the overall changes.
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
2 times, most recently
from
d6ffab9 to
f829a42
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
src/service/search/cluster/cacher.rs (3)
52-52: Avoid mutable variable declaration outside the loop.The
ts_columnvariable is declared outside the loop but never mutated. Consider moving it inside the loop.- let ts_column = cache_req.ts_column;
66-66: Avoid unnecessary cloning.The
ts_columnvariable is already aString, so callingto_stringis redundant.- let ts_column = ts_column.to_string();
210-217: Avoid redundant field assignments.The fields of
CacheQueryRequestare being reassigned to the same values. Consider directly passingcache_req.- CacheQueryRequest { - q_start_time: cache_req.q_start_time, - q_end_time: cache_req.q_end_time, - is_aggregate: cache_req.is_aggregate, - ts_column, - discard_interval: cache_req.discard_interval, - is_descending: cache_req.is_descending, - }, + cache_req,
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/common/meta/search.rs (1 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (11 hunks)
- src/infra/src/cache/file_data/disk.rs (1 hunks)
- src/infra/src/cache/meta.rs (1 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (8 hunks)
- src/service/search/cluster/cacher.rs (7 hunks)
Files skipped from review as they are similar to previous changes (6)
- src/common/meta/search.rs
- src/handler/grpc/request/query_cache.rs
- src/handler/http/request/search/mod.rs
- src/infra/src/cache/file_data/disk.rs
- src/infra/src/cache/meta.rs
- src/proto/proto/cluster/querycache.proto
Additional context used
Path-based instructions (2)
src/service/search/cluster/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cache/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (18)
src/service/search/cluster/cacher.rs (5)
22-25: Imports look good.The updated imports align with the new
CacheQueryRequeststruct.
31-31: Parameter update is correct.The function now accepts
CacheQueryRequestinstead of individual parameters, improving readability and maintainability.
70-78: Parameter mapping looks correct.The mapping of
CacheQueryRequestfields toQueryCacheRequestis appropriate.
175-182: New fields inCachedQueryResponseare correctly initialized.The new fields
is_descendingandlimitare correctly initialized in theCachedQueryResponse.
229-234: Conditional check and max_by_key logic are correct.The logic ensures there is an overlap between cache time range and query time range.
src/service/search/cache/cacher.rs (13)
29-29: Imports look good.The updated imports align with the new
CacheQueryRequeststruct.
60-61: Order by logic is correctly placed.The order by logic is correctly placed to determine sorting order.
115-116: Initializeis_descendingbased on order_by field.The
is_descendingvariable is initialized correctly based on theorder_byfield.
117-123: Order by loop is correct.The loop correctly sets the
is_descendingflag based on theorder_byfield.
129-136: Parameter mapping looks correct.The mapping of
CacheQueryRequestfields is appropriate.
177-182: New fields inCachedQueryResponseare correctly initialized.The new fields
is_descendingandlimitare correctly initialized in theCachedQueryResponse.
186-186: Limit field is correctly set.The
limitfield is correctly set based on the meta information.
194-194: Parameter update is correct.The function now accepts
CacheQueryRequestinstead of individual parameters, improving readability and maintainability.
207-213: Conditional check and max_by_key logic are correct.The logic ensures there is an overlap between cache time range and query time range.
216-220: File name generation is correct.The file name generation logic correctly incorporates the new
is_descendingfield.
271-276: Retain logic is correct.The logic to retain hits based on timestamp is correct.
291-298: New fields inCachedQueryResponseare correctly initialized.The new fields
is_descendingandlimitare correctly initialized in theCachedQueryResponse.
147-148: New fieldis_descendingis correctly added.The new field
is_descendingis correctly added to theResultCacheMetastruct.
| let mut deltas = vec![]; | ||
| calculate_deltas_v1( | ||
| &ResultCacheMeta { | ||
| start_time: cached_resp.response_start_time, | ||
| end_time: cached_resp.response_end_time, | ||
| is_aggregate, | ||
| is_descending: true, | ||
| }, | ||
| req.query.start_time, | ||
| req.query.end_time, | ||
| &mut deltas, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Remove redundant delta calculation.
The calculate_deltas_v1 function is used to calculate deltas but the result is not used in the subsequent code.
- let mut deltas = vec![];
- calculate_deltas_v1(
- &ResultCacheMeta {
- start_time: cached_resp.response_start_time,
- end_time: cached_resp.response_end_time,
- is_aggregate,
- is_descending: true,
- },
- req.query.start_time,
- req.query.end_time,
- &mut deltas,
- );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.
| let mut deltas = vec![]; | |
| calculate_deltas_v1( | |
| &ResultCacheMeta { | |
| start_time: cached_resp.response_start_time, | |
| end_time: cached_resp.response_end_time, | |
| is_aggregate, | |
| is_descending: true, | |
| }, | |
| req.query.start_time, | |
| req.query.end_time, | |
| &mut deltas, | |
| ); |
src/service/search/cache/cacher.rs
Outdated
| let discard_ts = if cache_req.is_descending { | ||
| let first_ts = get_ts_value( | ||
| &cache_req.ts_column, | ||
| cached_response.hits.first().unwrap(), | ||
| ); | ||
| if cache_req.discard_interval > 0 { | ||
| first_ts | ||
| } | ||
| } else if first_ts < last_ts { | ||
| // non-aggregate quer | ||
| let m_last_ts = round_down_to_nearest_minute(last_ts); | ||
| if Utc::now().timestamp_micros() - discard_duration < last_ts { | ||
| m_last_ts - discard_duration | ||
| } else { | ||
| matching_cache_meta.end_time | ||
| // non-aggregate query | ||
| let m_first_ts = round_down_to_nearest_minute(first_ts); | ||
| if Utc::now().timestamp_micros() - discard_duration < m_first_ts { | ||
| m_first_ts - discard_duration | ||
| } else { | ||
| m_first_ts | ||
| } | ||
| } | ||
| } else { | ||
| let m_first_ts = round_down_to_nearest_minute(first_ts); | ||
| if Utc::now().timestamp_micros() - discard_duration < m_first_ts { | ||
| m_first_ts - discard_duration | ||
| let last_ts = get_ts_value( | ||
| &cache_req.ts_column, | ||
| cached_response.hits.last().unwrap(), | ||
| ); | ||
| if cache_req.discard_interval > 0 { | ||
| last_ts | ||
| } else { | ||
| matching_cache_meta.start_time | ||
| } | ||
| }; | ||
|
|
||
| if !remove_hits.is_empty() { | ||
| for delta in remove_hits { | ||
| for hit in &cached_response.hits { | ||
| let hit_ts = get_ts_value(result_ts_column, hit); | ||
|
|
||
| if !(hit_ts >= delta.delta_start_time | ||
| && hit_ts < delta.delta_end_time) | ||
| && (hit_ts <= end_time && hit_ts >= start_time) | ||
| && hit_ts < discard_ts | ||
| { | ||
| to_retain.push(hit.clone()); | ||
| } | ||
| // non-aggregate query | ||
| let m_last_ts = round_down_to_nearest_minute(last_ts); | ||
| if Utc::now().timestamp_micros() - discard_duration < last_ts { | ||
| m_last_ts - discard_duration | ||
| } else { | ||
| m_last_ts |
There was a problem hiding this comment.
Optimize hit retention logic.
The hit retention logic can be optimized by using retain instead of creating a new vector.
- let discard_ts = if cache_req.is_descending {
- let first_ts = get_ts_value(&cache_req.ts_column, cached_response.hits.first().unwrap());
- if cache_req.discard_interval > 0 {
- first_ts
- } else {
- let m_first_ts = round_down_to_nearest_minute(first_ts);
- if Utc::now().timestamp_micros() - discard_duration < m_first_ts {
- m_first_ts - discard_duration
- } else {
- m_first_ts
- }
- }
- } else {
- let last_ts = get_ts_value(&cache_req.ts_column, cached_response.hits.last().unwrap());
- if cache_req.discard_interval > 0 {
- last_ts
- } else {
- let m_last_ts = round_down_to_nearest_minute(last_ts);
- if Utc::now().timestamp_micros() - discard_duration < last_ts {
- m_last_ts - discard_duration
- } else {
- m_last_ts
- }
- }
- };
- cached_response.hits.retain(|hit| {
- let hit_ts = get_ts_value(&cache_req.ts_column, hit);
- hit_ts <= cache_req.q_end_time && hit_ts >= cache_req.q_start_time && hit_ts < discard_ts
- });
+ let discard_ts = if cache_req.is_descending {
+ let first_ts = get_ts_value(&cache_req.ts_column, cached_response.hits.first().unwrap());
+ if cache_req.discard_interval > 0 {
+ first_ts
+ } else {
+ let m_first_ts = round_down_to_nearest_minute(first_ts);
+ if Utc::now().timestamp_micros() - discard_duration < m_first_ts {
+ m_first_ts - discard_duration
+ } else {
+ m_first_ts
+ }
+ }
+ } else {
+ let last_ts = get_ts_value(&cache_req.ts_column, cached_response.hits.last().unwrap());
+ if cache_req.discard_interval > 0 {
+ last_ts
+ } else {
+ let m_last_ts = round_down_to_nearest_minute(last_ts);
+ if Utc::now().timestamp_micros() - discard_duration < last_ts {
+ m_last_ts - discard_duration
+ } else {
+ m_last_ts
+ }
+ }
+ };
+ cached_response.hits.retain(|hit| {
+ let hit_ts = get_ts_value(&cache_req.ts_column, hit);
+ hit_ts <= cache_req.q_end_time && hit_ts >= cache_req.q_start_time && hit_ts < discard_ts
+ });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.
| let discard_ts = if cache_req.is_descending { | |
| let first_ts = get_ts_value( | |
| &cache_req.ts_column, | |
| cached_response.hits.first().unwrap(), | |
| ); | |
| if cache_req.discard_interval > 0 { | |
| first_ts | |
| } | |
| } else if first_ts < last_ts { | |
| // non-aggregate quer | |
| let m_last_ts = round_down_to_nearest_minute(last_ts); | |
| if Utc::now().timestamp_micros() - discard_duration < last_ts { | |
| m_last_ts - discard_duration | |
| } else { | |
| matching_cache_meta.end_time | |
| // non-aggregate query | |
| let m_first_ts = round_down_to_nearest_minute(first_ts); | |
| if Utc::now().timestamp_micros() - discard_duration < m_first_ts { | |
| m_first_ts - discard_duration | |
| } else { | |
| m_first_ts | |
| } | |
| } | |
| } else { | |
| let m_first_ts = round_down_to_nearest_minute(first_ts); | |
| if Utc::now().timestamp_micros() - discard_duration < m_first_ts { | |
| m_first_ts - discard_duration | |
| let last_ts = get_ts_value( | |
| &cache_req.ts_column, | |
| cached_response.hits.last().unwrap(), | |
| ); | |
| if cache_req.discard_interval > 0 { | |
| last_ts | |
| } else { | |
| matching_cache_meta.start_time | |
| } | |
| }; | |
| if !remove_hits.is_empty() { | |
| for delta in remove_hits { | |
| for hit in &cached_response.hits { | |
| let hit_ts = get_ts_value(result_ts_column, hit); | |
| if !(hit_ts >= delta.delta_start_time | |
| && hit_ts < delta.delta_end_time) | |
| && (hit_ts <= end_time && hit_ts >= start_time) | |
| && hit_ts < discard_ts | |
| { | |
| to_retain.push(hit.clone()); | |
| } | |
| // non-aggregate query | |
| let m_last_ts = round_down_to_nearest_minute(last_ts); | |
| if Utc::now().timestamp_micros() - discard_duration < last_ts { | |
| m_last_ts - discard_duration | |
| } else { | |
| m_last_ts | |
| let discard_ts = if cache_req.is_descending { | |
| let first_ts = get_ts_value( | |
| &cache_req.ts_column, | |
| cached_response.hits.first().unwrap(), | |
| ); | |
| if cache_req.discard_interval > 0 { | |
| first_ts | |
| } else { | |
| let m_first_ts = round_down_to_nearest_minute(first_ts); | |
| if Utc::now().timestamp_micros() - discard_duration < m_first_ts { | |
| m_first_ts - discard_duration | |
| } else { | |
| m_first_ts | |
| } | |
| } | |
| } else { | |
| let last_ts = get_ts_value( | |
| &cache_req.ts_column, | |
| cached_response.hits.last().unwrap(), | |
| ); | |
| if cache_req.discard_interval > 0 { | |
| last_ts | |
| } else { | |
| let m_last_ts = round_down_to_nearest_minute(last_ts); | |
| if Utc::now().timestamp_micros() - discard_duration < last_ts { | |
| m_last_ts - discard_duration | |
| } else { | |
| m_last_ts | |
| } | |
| } | |
| }; | |
| cached_response.hits.retain(|hit| { | |
| let hit_ts = get_ts_value(&cache_req.ts_column, hit); | |
| hit_ts <= cache_req.q_end_time && hit_ts >= cache_req.q_start_time && hit_ts < discard_ts | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
src/common/utils/http.rs (1)
Line range hint
99-130:
Remove redundant check forspan.is_none().Since
spanis no longer anOption, the check forspan.is_none()is redundant and should be removed.- if !span.is_none() { + {src/handler/http/request/search/multi_streams.rs (3)
122-124: Ensure proper usage of tracing spans.The tracing span is correctly created and used based on the configuration. However, consider adding a more descriptive name for the span to improve trace readability.
tracing::info_span!("search_multi_api", org_id = org_id.clone())
426-429: Ensure proper usage of tracing spans.The tracing span is correctly created and used based on the configuration. However, consider adding a more descriptive name for the span to improve trace readability.
tracing::info_span!("search_partition_multi_api", org_id = org_id.clone())
567-571: Ensure proper usage of tracing spans.The tracing span is correctly created and used based on the configuration. However, consider adding a more descriptive name for the span to improve trace readability.
tracing::info_span!("around_multi_api", org_id = org_id.clone(), stream_names = stream_names.clone())
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/common/utils/http.rs (3 hunks)
- src/handler/http/request/search/mod.rs (26 hunks)
- src/handler/http/request/search/multi_streams.rs (8 hunks)
- src/handler/http/request/stream/mod.rs (1 hunks)
- src/handler/http/request/traces/mod.rs (4 hunks)
- src/service/search/cache/cacher.rs (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/request/search/mod.rs
Additional context used
Path-based instructions (5)
src/common/utils/http.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/stream/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cache/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/traces/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/search/multi_streams.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (11)
src/handler/http/request/stream/mod.rs (1)
321-326: LGTM!The addition of the check for
result_cache_enabledensures that the function respects the configuration setting.src/service/search/cache/cacher.rs (5)
115-123: LGTM!The changes correctly handle the
is_descendingfield in thecheck_cachefunction.
129-136: LGTM!The
CacheQueryRequeststruct initialization correctly includes theis_descendingfield.
141-153: Remove redundant delta calculation.The
calculate_deltas_v1function is used to calculate deltas but the result is not used in the subsequent code.- let mut deltas = vec![]; - calculate_deltas_v1( - &ResultCacheMeta { - start_time: cached_resp.response_start_time, - end_time: cached_resp.response_end_time, - is_aggregate, - is_descending: true, - }, - req.query.start_time, - req.query.end_time, - &mut deltas, - );
179-182: LGTM!The
CachedQueryResponsestruct initialization correctly includes theis_descendingfield.
Line range hint
195-298:
LGTM!The changes correctly handle the
is_descendingfield in theget_cached_resultsfunction.src/handler/http/request/traces/mod.rs (1)
136-144: LGTM!The function call to
get_or_create_trace_idcorrectly uses the new&tracing::Spantype.src/handler/http/request/search/multi_streams.rs (4)
262-262: Ensure consistency in tracing configuration.The condition
if !cfg.common.tracing_enabled && cfg.common.tracing_search_enabledmight be confusing. Ensure that this condition is intentional and correctly reflects the intended tracing behavior.
449-449: Ensure consistency in tracing configuration.The condition
if !cfg.common.tracing_enabled && cfg.common.tracing_search_enabledmight be confusing. Ensure that this condition is intentional and correctly reflects the intended tracing behavior.
717-717: Ensure consistency in tracing configuration.The condition
if !cfg.common.tracing_enabled && cfg.common.tracing_search_enabledmight be confusing. Ensure that this condition is intentional and correctly reflects the intended tracing behavior.
794-794: Ensure consistency in tracing configuration.The condition
if !cfg.common.tracing_enabled && cfg.common.tracing_search_enabledmight be confusing. Ensure that this condition is intentional and correctly reflects the intended tracing behavior.
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
from
e9fcbc9 to
e605bfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
src/service/search/cluster/cacher.rs (3)
52-52: Useto_stringonly when necessary.The
ts_columnis cloned and converted to a string multiple times. This can be optimized.- let ts_column = cache_req.ts_column; + let ts_column = cache_req.ts_column.clone();
66-66: Avoid redundantto_string()calls.The
ts_columnis already aStringand does not need to be converted again.- let ts_column = ts_column.to_string(); + let ts_column = ts_column.clone();
229-234: Optimize range filtering logic.The range filtering logic can be optimized for better readability and performance.
- match results - .iter() - .filter(|(_, cache_meta)| { - cache_meta.response_start_time <= cache_req.q_end_time - && cache_meta.response_end_time >= cache_req.q_start_time - }) - .max_by_key(|(_, result)| { - result.response_end_time.min(cache_req.q_end_time) - - result.response_start_time.max(cache_req.q_start_time) - }) { + match results.iter().filter(|(_, cache_meta)| { + cache_meta.response_start_time <= cache_req.q_end_time + && cache_meta.response_end_time >= cache_req.q_start_time + }).max_by_key(|(_, result)| { + result.response_end_time.min(cache_req.q_end_time) + - result.response_start_time.max(cache_req.q_start_time) + }) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- src/common/meta/search.rs (1 hunks)
- src/common/utils/http.rs (3 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (26 hunks)
- src/handler/http/request/search/multi_streams.rs (8 hunks)
- src/handler/http/request/stream/mod.rs (1 hunks)
- src/handler/http/request/traces/mod.rs (4 hunks)
- src/infra/src/cache/file_data/disk.rs (1 hunks)
- src/infra/src/cache/meta.rs (1 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (8 hunks)
- src/service/search/cluster/cacher.rs (7 hunks)
Files skipped from review as they are similar to previous changes (10)
- src/common/meta/search.rs
- src/common/utils/http.rs
- src/handler/grpc/request/query_cache.rs
- src/handler/http/request/search/mod.rs
- src/handler/http/request/stream/mod.rs
- src/handler/http/request/traces/mod.rs
- src/infra/src/cache/file_data/disk.rs
- src/infra/src/cache/meta.rs
- src/proto/proto/cluster/querycache.proto
- src/service/search/cache/cacher.rs
Additional context used
Path-based instructions (2)
src/service/search/cluster/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/search/multi_streams.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (12)
src/service/search/cluster/cacher.rs (5)
22-25: Ensure proper integration of new imports.The new imports from
common::meta::searchshould be correctly integrated and used within the module.
78-78: Ensureis_descendingis correctly utilized.The
is_descendingfield is correctly passed to theQueryCacheRequest. Ensure that all necessary logic for handling this field is implemented.
175-182: Ensure consistency in field initialization.The
CachedQueryResponsestruct initialization should be consistent. Verify that all fields are correctly initialized.
210-217: Verify local cache query request.Ensure that the local cache query request correctly initializes the
CacheQueryRequestwith the newis_descendingfield.
22-25: Confirm function logic.Ensure that the function logic for
delete_cached_resultsis optimal and there are no hidden issues.src/handler/http/request/search/multi_streams.rs (7)
122-124: Ensure correct integration of tracing spans.The new tracing spans should be correctly integrated and utilized within the function.
262-262: Verify tracing instrumentation logic.Ensure that the logic for tracing instrumentation is correctly implemented and does not introduce performance overhead.
426-431: Ensure correct integration of tracing spans.The new tracing spans should be correctly integrated and utilized within the function.
449-449: Verify tracing instrumentation logic.Ensure that the logic for tracing instrumentation is correctly implemented and does not introduce performance overhead.
567-573: Ensure correct integration of tracing spans.The new tracing spans should be correctly integrated and utilized within the function.
717-717: Verify tracing instrumentation logic.Ensure that the logic for tracing instrumentation is correctly implemented and does not introduce performance overhead.
794-794: Verify tracing instrumentation logic.Ensure that the logic for tracing instrumentation is correctly implemented and does not introduce performance overhead.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (2)
- Cargo.toml (3 hunks)
- src/handler/http/request/search/mod.rs (25 hunks)
Additional context used
Path-based instructions (1)
src/handler/http/request/search/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (8)
Cargo.toml (2)
25-26: LGTM! New feature additions are appropriate.The new default feature
enterpriseand its dependency are correctly added.
60-60: LGTM! New dependency additions are appropriate.The new dependencies
actix-wsando2_enterpriseare correctly added.Also applies to: 103-103
src/handler/http/request/search/mod.rs (6)
48-49: LGTM! Import changes are appropriate.The imports
get_or_create_trace_id,get_search_type_from_request, andget_stream_type_from_requestare necessary for the updated functionality.
285-289: LGTM! Improved logging.The updated logging blocks provide better instrumentation and debugging information.
Line range hint
339-351: LGTM! Task spawning updates are appropriate.The updated task spawning logic ensures better handling of cached responses, improving performance.
Also applies to: 359-369
419-424: LGTM! Cache handling updates are appropriate.The updates in the
merge_responsefunction ensure proper handling of cache limits and result ratios.
558-564: LGTM! Tracing span updates are appropriate.The updated tracing spans in the
aroundfunction provide better instrumentation and debugging information.
983-985: LGTM! Updates invalues_v1are appropriate.The updates in the
values_v1function ensure better handling of tracing spans and improved search logic.Also applies to: 1155-1164
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
from
054eec0 to
1536bd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (13)
- Cargo.toml (3 hunks)
- src/common/meta/search.rs (1 hunks)
- src/common/utils/http.rs (3 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (24 hunks)
- src/handler/http/request/search/multi_streams.rs (8 hunks)
- src/handler/http/request/stream/mod.rs (1 hunks)
- src/handler/http/request/traces/mod.rs (4 hunks)
- src/infra/src/cache/file_data/disk.rs (1 hunks)
- src/infra/src/cache/meta.rs (1 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (8 hunks)
- src/service/search/cluster/cacher.rs (7 hunks)
Files skipped from review as they are similar to previous changes (12)
- Cargo.toml
- src/common/meta/search.rs
- src/common/utils/http.rs
- src/handler/grpc/request/query_cache.rs
- src/handler/http/request/search/multi_streams.rs
- src/handler/http/request/stream/mod.rs
- src/handler/http/request/traces/mod.rs
- src/infra/src/cache/file_data/disk.rs
- src/infra/src/cache/meta.rs
- src/proto/proto/cluster/querycache.proto
- src/service/search/cache/cacher.rs
- src/service/search/cluster/cacher.rs
Additional context used
Path-based instructions (1)
src/handler/http/request/search/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (6)
src/handler/http/request/search/mod.rs (6)
285-289: Approved: Added logging statements.The added logging statements improve traceability and are a good practice.
348-351: Approved: Added logging statements for conditional checks.The added logging statements improve traceability and are a good practice.
558-564: Approved: Added logging statements.The added logging statements improve traceability and are a good practice.
899-905: Approved: Added logging statements.The added logging statements improve traceability and are a good practice.
1155-1164: Approved: Added logging statements.The added logging statements improve traceability and are a good practice.
1361-1370: Approved: Added logging statements.The added logging statements improve traceability and are a good practice.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/search/mod.rs (1 hunks)
Additional context used
Path-based instructions (1)
src/service/search/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (1)
src/service/search/mod.rs (1)
Line range hint
58-58:
Consider the impact of removing thetracing::instrumentattribute.The removal of the
tracing::instrumentattribute affects the tracing instrumentation of thesearchfunction. This might impact the observability and debugging capabilities. Re-evaluate if this change is necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/handler/http/request/search/mod.rs (24 hunks)
- src/handler/http/request/search/multi_streams.rs (8 hunks)
- src/handler/http/request/traces/mod.rs (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/handler/http/request/search/mod.rs
- src/handler/http/request/search/multi_streams.rs
- src/handler/http/request/traces/mod.rs
oasisk
force-pushed
the
fix-query-result-cache-v2
branch
from
7fb4b3a to
da217db
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (15)
- .github/workflows/unit-tests.yml (1 hunks)
- rust-toolchain.toml (1 hunks)
- src/common/meta/search.rs (1 hunks)
- src/common/utils/http.rs (3 hunks)
- src/handler/grpc/request/query_cache.rs (2 hunks)
- src/handler/http/request/search/mod.rs (24 hunks)
- src/handler/http/request/search/multi_streams.rs (8 hunks)
- src/handler/http/request/stream/mod.rs (1 hunks)
- src/handler/http/request/traces/mod.rs (4 hunks)
- src/infra/src/cache/file_data/disk.rs (1 hunks)
- src/infra/src/cache/meta.rs (1 hunks)
- src/proto/proto/cluster/querycache.proto (1 hunks)
- src/service/search/cache/cacher.rs (8 hunks)
- src/service/search/cluster/cacher.rs (7 hunks)
- src/service/search/mod.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/unit-tests.yml
- rust-toolchain.toml
Files skipped from review as they are similar to previous changes (8)
- src/common/meta/search.rs
- src/common/utils/http.rs
- src/handler/grpc/request/query_cache.rs
- src/handler/http/request/stream/mod.rs
- src/handler/http/request/traces/mod.rs
- src/infra/src/cache/meta.rs
- src/proto/proto/cluster/querycache.proto
- src/service/search/mod.rs
Additional context used
Path-based instructions (5)
src/service/search/cluster/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/cache/cacher.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/infra/src/cache/file_data/disk.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/search/multi_streams.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/handler/http/request/search/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (33)
src/service/search/cluster/cacher.rs (2)
78-78: LGTM! Approved changes foris_descending.The addition of
is_descendingtoQueryCacheRequestis correctly integrated here.
181-182: Check the default value forlimit.Ensure that the default value of
-1forlimitis appropriate and doesn't cause unintended side effects.src/service/search/cache/cacher.rs (3)
115-116: LGTM! Approved changes foris_descending.The addition of
is_descendingtocheck_cacheis correctly integrated here.
297-297: LGTM! Approved changes foris_descending.The addition of
is_descendingtoCachedQueryResponseis correctly integrated here.
181-182: Check the default value forlimit.Ensure that the default value of
-1forlimitis appropriate and doesn't cause unintended side effects.src/infra/src/cache/file_data/disk.rs (1)
415-415: LGTM! Approved changes foris_descending.The addition of
is_descendingtoloadis correctly integrated here.src/handler/http/request/search/multi_streams.rs (8)
31-31: Ensuretracingis necessary.The
use tracing::{Instrument, Span};statement is added. Ensure that tracing is enabled and necessary for this module.
122-124: LGTM!The tracing span
http_spanis correctly created based on the configuration.
253-261: Instrument the search service call.The search service call is correctly instrumented with the tracing span.
422-427: LGTM!The tracing span
http_spanis correctly created based on the configuration.
445-445: Instrument the search service call based on tracing configuration.The search service call is correctly instrumented with the tracing span based on the tracing configuration.
563-569: LGTM!The tracing span
http_spanis correctly created based on the configuration.
710-714: Instrument the search service call.The search service call is correctly instrumented with the tracing span.
785-789: Instrument the search service call.The search service call is correctly instrumented with the tracing span.
src/handler/http/request/search/mod.rs (19)
146-148: LGTM!The tracing span is correctly set up for the search function.
285-289: LGTM!The logging statements for query start and end times provide useful information for debugging.
348-351: LGTM!The logging statements within the async task are correctly formatted and provide useful information.
365-367: LGTM!The instrumentation for the search function call is correctly set up.
419-424: LGTM!The call to the
merge_responsefunction is correctly set up with the appropriate arguments.
495-495: LGTM!The call to the
write_resultsfunction is correctly set up with the appropriate arguments.
558-564: LGTM!The tracing span is correctly set up for the around function.
694-697: LGTM!The instrumentation for the search function call within the around function is correctly set up.
747-750: LGTM!The instrumentation for the search function call within the around function is correctly set up.
897-903: LGTM!The tracing span is correctly set up for the values function.
981-984: LGTM!The instrumentation for the search function call within the values_v1 function is correctly set up.
1153-1162: LGTM!The instrumentation for the search function call within the values_v1 function is correctly set up.
1251-1251: LGTM!The instrumentation for the search function call within the values_v2 function is correctly set up.
1312-1319: LGTM!The logging statements for query wait time within the async task are correctly formatted and provide useful information.
1359-1368: LGTM!The instrumentation for the search function call within the values_v2 function is correctly set up.
1485-1487: LGTM!The tracing span is correctly set up for the search_partition function.
1506-1508: LGTM!The instrumentation for the search function call within the search_partition function is correctly set up.
Line range hint
1538-1597:
LGTM!The call to the
merge_responsefunction is correctly set up with the appropriate arguments.
Line range hint
1600-1656:
LGTM!The call to the
write_resultsfunction is correctly set up with the appropriate arguments.
| trace_id: String, | ||
| result_ts_column: String, | ||
| discard_interval: i64, | ||
| cache_req: CacheQueryRequest, |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure consistent usage of CacheQueryRequest.
The is_descending field is missing in the following instances of CacheQueryRequest construction:
src/service/search/cache/cacher.rssrc/service/search/cluster/cacher.rssrc/handler/grpc/request/query_cache.rs
Please update these instances to include the is_descending field.
Analysis chain
Ensure consistent usage of CacheQueryRequest.
Verify that all instances where CacheQueryRequest is constructed include the new is_descending field.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all instances of `CacheQueryRequest` usage include `is_descending`.
# Test: Search for `CacheQueryRequest` construction. Expect: All instances include `is_descending`.
rg --type rust $'CacheQueryRequest {'
Length of output: 140
Script:
#!/bin/bash
# Description: Verify all instances of `CacheQueryRequest` usage include `is_descending`.
# Test: Search for `CacheQueryRequest` construction. Expect: All instances include `is_descending`.
rg 'CacheQueryRequest \{' -A 5
Length of output: 1785
| ts_column: result_ts_col.clone(), | ||
| discard_interval, | ||
| is_descending, | ||
| }, |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure consistent usage of is_descending.
The following instances of CacheQueryRequest construction are missing the is_descending field:
src/service/search/cache/cacher.rssrc/service/search/cluster/cacher.rssrc/handler/grpc/request/query_cache.rs
Please update these instances to include the is_descending field.
Analysis chain
Ensure consistent usage of is_descending.
Verify that all instances where CacheQueryRequest is constructed include the new is_descending field.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all instances of `CacheQueryRequest` usage include `is_descending`.
# Test: Search for `CacheQueryRequest` construction. Expect: All instances include `is_descending`.
rg --type rust $'CacheQueryRequest {'
Length of output: 140
Script:
#!/bin/bash
# Description: Verify all instances of `CacheQueryRequest` usage include `is_descending`.
# Test: Search for `CacheQueryRequest` construction. Expect: All instances include `is_descending`.
rg --type rust 'CacheQueryRequest \{' -A 5
Length of output: 1797
| result_ts_column: &str, | ||
| trace_id: &str, | ||
| discard_interval: i64, | ||
| cache_req: CacheQueryRequest, |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure consistent usage of CacheQueryRequest.
The following instances of CacheQueryRequest construction do not include the is_descending field:
src/service/search/cache/cacher.rssrc/service/search/cluster/cacher.rssrc/handler/grpc/request/query_cache.rs
Please update these instances to include the is_descending field.
Analysis chain
Ensure consistent usage of CacheQueryRequest.
Verify that all instances where CacheQueryRequest is constructed include the new is_descending field.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all instances of `CacheQueryRequest` usage include `is_descending`.
# Test: Search for `CacheQueryRequest` construction. Expect: All instances include `is_descending`.
rg --type rust $'CacheQueryRequest {'
Length of output: 140
Script:
#!/bin/bash
# Description: Verify all instances of `CacheQueryRequest` usage include `is_descending`.
# Test: Search for `CacheQueryRequest` construction. Expect: All instances include `is_descending`.
rg --type rust 'CacheQueryRequest \{'
Length of output: 296
Script:
#!/bin/bash
# Description: Verify all instances of `CacheQueryRequest` usage include `is_descending`.
# Test: Check instances in `src/service/search/cache/cacher.rs`
rg -A 5 'CacheQueryRequest \{' src/service/search/cache/cacher.rs
# Test: Check instances in `src/service/search/cluster/cacher.rs`
rg -A 5 'CacheQueryRequest \{' src/service/search/cluster/cacher.rs
# Test: Check instances in `src/handler/grpc/request/query_cache.rs`
rg -A 5 'CacheQueryRequest \{' src/handler/grpc/request/query_cache.rs
Length of output: 957
| start_time: meta[0].parse().unwrap(), | ||
| end_time: meta[1].parse().unwrap(), | ||
| is_aggregate, | ||
| is_descending, |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure consistent usage of is_descending.
The is_descending field is missing in some instances of ResultCacheMeta construction. Please include the is_descending field in the following locations:
src/service/search/cache/cacher.rssrc/handler/http/request/search/mod.rs
Analysis chain
Ensure consistent usage of is_descending.
Verify that all instances where ResultCacheMeta is constructed include the new is_descending field.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all instances of `ResultCacheMeta` usage include `is_descending`.
# Test: Search for `ResultCacheMeta` construction. Expect: All instances include `is_descending`.
rg --type rust $'ResultCacheMeta {'
Length of output: 134
Script:
#!/bin/bash
# Description: Verify all instances of `ResultCacheMeta` usage include `is_descending`.
# Test: Search for `ResultCacheMeta` construction. Expect: All instances include `is_descending`.
rg --type rust 'ResultCacheMeta \{'
Length of output: 331
Script:
#!/bin/bash
# Description: Verify all instances of `ResultCacheMeta` usage include `is_descending`.
# Extract context around each instance of `ResultCacheMeta` construction
rg --type rust -C 3 'ResultCacheMeta \{' src/service/search/cache/cacher.rs
rg --type rust -C 3 'ResultCacheMeta \{' src/infra/src/cache/meta.rs
rg --type rust -C 3 'ResultCacheMeta \{' src/infra/src/cache/file_data/disk.rs
rg --type rust -C 3 'ResultCacheMeta \{' src/handler/http/request/search/mod.rs
Length of output: 1569
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added new fields `is_descending` and `limit` to query responses for enhanced sorting and pagination. - **Improvements** - Enhanced logging and tracing for better query time tracking and debugging. - Streamlined span handling for more efficient tracing and instrumentation. - **Bug Fixes** - Fixed validation for `result_cache_enabled` configuration to prevent cache-related errors. - Improved accuracy in cache limit handling in query responses. - **Chores** - Updated Rust toolchain version for improved compatibility and performance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
is_descendingandlimitto query responses for enhanced sorting and pagination.Improvements
Bug Fixes
result_cache_enabledconfiguration to prevent cache-related errors.Chores