fix: Revert change storage ttl time from seconds to milliseconds and clear space in CI after compile #2853
Conversation
WalkthroughThe recent modifications enhance the CI workflow by adding a new "Cleanup" step after build commands across multiple job definitions. This step ensures the removal of temporary build artifacts and dependencies, fostering a cleaner environment that boosts efficiency and prevents conflicts in future builds. Additionally, several parameter names have been standardized for clarity, particularly regarding time-to-live (TTL) functionalities, improving overall code readability. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as Continuous Integration
participant JobA as Job A
participant JobB as Job B
participant Cleanup as Cleanup Step
CI->>JobA: Start Build
JobA->>Cleanup: Execute Build Commands
Cleanup-->>JobA: Cleanup Completed
CI->>JobB: Start Build
JobB->>Cleanup: Execute Build Commands
Cleanup-->>JobB: Cleanup Completed
Poem
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 Configuration File (
|
github-actions
bot
added
☢️ Bug
![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 (1)
- .github/workflows/pika.yml (3 hunks)
Additional comments not posted (3)
.github/workflows/pika.yml (3)
48-51: LGTM! The Cleanup step is correctly implemented.The addition of the "Cleanup" step effectively removes temporary build artifacts, ensuring a clean environment for future builds.
127-130: Consistent Cleanup step implementation.The "Cleanup" step is consistent with other jobs, ensuring uniformity in the CI workflow.
188-191: Uniform Cleanup step across platforms.The "Cleanup" step is consistently applied across all job definitions, ensuring a clean and efficient CI process.
cheniujh
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
src/storage/src/redis_strings.cc (2)
Line range hint
534-590: Refactor TTL calculation.Consider using
CalculateTTLto avoid repeating TTL calculation logic and enhance consistency.- *ttl = parsed_strings_value.Etime(); - if (*ttl == 0) { - *ttl = -1; - } else { - int64_t curtime; - rocksdb::Env::Default()->GetCurrentTime(&curtime); - *ttl = *ttl - curtime >= 0 ? *ttl - curtime : -2; - } + *ttl = CalculateTTL(parsed_strings_value.Etime());
Line range hint
1446-1483: Refactor TTL calculation.Consider using
CalculateTTLto avoid repeating TTL calculation logic and enhance consistency.- *timestamp = parsed_strings_value.Etime(); - if (*timestamp == 0) { - *timestamp = -1; - } else { - int64_t curtime; - rocksdb::Env::Default()->GetCurrentTime(&curtime); - *timestamp = *timestamp - curtime >= 0 ? *timestamp - curtime : -2; - } + *timestamp = CalculateTTL(parsed_strings_value.Etime());
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (38)
- .github/workflows/pika.yml (4 hunks)
- include/pika_cache.h (1 hunks)
- include/pika_kv.h (10 hunks)
- src/net/examples/performance/server.cc (1 hunks)
- src/pika_admin.cc (2 hunks)
- src/pika_bit.cc (1 hunks)
- src/pika_cache.cc (1 hunks)
- src/pika_cache_load_thread.cc (1 hunks)
- src/pika_conf.cc (2 hunks)
- src/pika_geo.cc (1 hunks)
- src/pika_kv.cc (32 hunks)
- src/pika_list.cc (1 hunks)
- src/pika_server.cc (5 hunks)
- src/pika_zset.cc (5 hunks)
- src/pstd/include/env.h (2 hunks)
- src/pstd/src/env.cc (1 hunks)
- src/storage/include/storage/storage.h (15 hunks)
- src/storage/src/base_data_value_format.h (4 hunks)
- src/storage/src/base_filter.h (2 hunks)
- src/storage/src/base_meta_value_format.h (5 hunks)
- src/storage/src/base_value_format.h (4 hunks)
- src/storage/src/lists_filter.h (1 hunks)
- src/storage/src/lists_meta_value_format.h (5 hunks)
- src/storage/src/redis.h (7 hunks)
- src/storage/src/redis_hashes.cc (16 hunks)
- src/storage/src/redis_lists.cc (9 hunks)
- src/storage/src/redis_sets.cc (15 hunks)
- src/storage/src/redis_streams.cc (1 hunks)
- src/storage/src/redis_strings.cc (22 hunks)
- src/storage/src/redis_zsets.cc (17 hunks)
- src/storage/src/storage.cc (16 hunks)
- src/storage/src/strings_filter.h (1 hunks)
- src/storage/src/strings_value_format.h (4 hunks)
- src/storage/src/zsets_filter.h (1 hunks)
- src/storage/tests/keys_test.cc (1 hunks)
- src/storage/tests/lists_filter_test.cc (2 hunks)
- src/storage/tests/strings_filter_test.cc (1 hunks)
- src/storage/tests/strings_test.cc (7 hunks)
Files skipped from review due to trivial changes (10)
- src/pika_admin.cc
- src/pika_bit.cc
- src/pika_conf.cc
- src/pika_geo.cc
- src/pika_list.cc
- src/pika_server.cc
- src/pika_zset.cc
- src/storage/src/redis_streams.cc
- src/storage/src/storage.cc
- src/storage/tests/strings_filter_test.cc
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pika.yml
Additional context used
Learnings (1)
src/storage/src/redis_sets.cc (1)
Learnt from: wangshao1 PR: OpenAtomFoundation/pika#2744 File: src/storage/src/redis_sets.cc:0-0 Timestamp: 2024-06-20T09:25:58.037Z Learning: Refactor involving extensive code changes should be deferred to subsequent versions to avoid risks before major releases. This is tracked under issue #2751 for the Pika project.
Additional comments not posted (144)
src/storage/src/strings_filter.h (1)
23-24: Verify the impact of time retrieval change.The method of retrieving the current time has been changed from
pstd::NowMillis()torocksdb::Env::Default()->GetCurrentTime(&unix_time). Ensure this change does not affect the precision or logic of theFiltermethod, especially in terms of time zone or granularity.src/net/examples/performance/server.cc (1)
89-89: Verify the impact of changing time variable types.The type of
standedhas been changed frompstd::TimeTypetouint64_t. Ensure this change does not affect the logic or precision of time calculations in the main function, especially in terms of overflow or compatibility with other time-related functions.Verification successful
Change to
uint64_tfor time variables is safe. The use ofuint64_tforstandedis consistent with theNowMicros()function's return type and supports the necessary precision for time calculations without risk of overflow.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and compatibility of `uint64_t` for time calculations in the codebase. # Test: Search for the usage of `pstd::TimeType` and `uint64_t` in time calculations. # Expect: Consistent usage and no overflow issues across the codebase. rg --type cpp 'pstd::TimeType' rg --type cpp 'uint64_t'Length of output: 90727
Script:
#!/bin/bash # Search for the usage of `st` and `ed` in the `server.cc` file to verify their involvement in time calculations or logic. rg 'st|ed' src/net/examples/performance/server.cc -A 5 -B 5Length of output: 2523
src/storage/src/base_data_value_format.h (1)
43-43: Verify the correctness ofctime_handling changes.The logic for handling
ctime_has been simplified by directly encoding and decoding it. Ensure these changes maintain the intended functionality and do not introduce any regressions, particularly in terms of data integrity and backward compatibility.Also applies to: 61-61, 73-73, 84-84
src/storage/src/base_value_format.h (4)
44-44: Improved time precision in constructor.The use of
pstd::NowMicros() / 1e6enhances the precision of thectime_initialization by using microseconds.
54-57: Consistent timestamp handling inSetRelativeTimestamp.The method now uses
rocksdb::Env::Default()->GetCurrentTime(&unix_time), aligning with the new timestamp logic.
125-128: Uniform timestamp handling inParsedInternalValue.The method mirrors the changes in
InternalValue, ensuring consistent timestamp handling.
138-139: Accurate time checks inIsStale.The method now uses
rocksdb::Env::Default()->GetCurrentTime(&unix_time), improving accuracy in expiration checks.src/pstd/include/env.h (2)
64-64: Simplified type usage inNowMicros.The function now directly returns
uint64_t, enhancing clarity by removing theTimeTypealias.
65-65: Consistent type usage inNowMillis.The function mirrors the change in
NowMicros, returninguint64_tdirectly for improved clarity.src/storage/src/strings_value_format.h (3)
38-40: Simplified timestamp encoding inStringsValue.The method now directly encodes
ctime_andetime_, reducing complexity and improving readability.
81-83: Simplified timestamp decoding inParsedStringsValue.The constructor now directly assigns
ctime_andetime_, enhancing clarity by removing conditional logic.
117-125: Direct timestamp encoding inParsedStringsValue.The methods now directly encode
ctime_andetime_, simplifying the logic and improving readability.src/storage/src/zsets_filter.h (1)
83-84: Correctly updated time retrieval method.The change from
pstd::NowMillis()torocksdb::Env::Default()->GetCurrentTime(&unix_time)is correctly implemented. This aligns with the shift towards using RocksDB's environment for time retrieval, which can enhance consistency across the codebase.src/storage/src/lists_filter.h (1)
91-92: Correctly updated time retrieval method.The change from
pstd::NowMillis()torocksdb::Env::Default()->GetCurrentTime(&unix_time)is correctly implemented. This aligns with the shift towards using RocksDB's environment for time retrieval, which can enhance consistency across the codebase.src/storage/src/base_meta_value_format.h (6)
42-44: Simplified timestamp encoding.The removal of conditional logic for encoding
ctime_andetime_simplifies the code and reduces potential errors related to timestamp handling. This change enhances clarity and maintainability.
49-53: Updated time retrieval method inUpdateVersion.The change from
pstd::NowMicros()torocksdb::Env::Default()->GetCurrentTime(&unix_time)inUpdateVersionis correctly implemented. This ensures that the versioning logic is based on a more precise time source.
74-76: Simplified timestamp decoding.The removal of conditional logic for decoding
ctime_andetime_simplifies the code and reduces potential errors related to timestamp handling. This change enhances clarity and maintainability.
94-96: Simplified timestamp decoding.The removal of conditional logic for decoding
ctime_andetime_simplifies the code and reduces potential errors related to timestamp handling. This change enhances clarity and maintainability.
117-124: Simplified timestamp setting methods.The methods
SetCtimeToValueandSetEtimeToValuehave been simplified to directly encode the timestamps, improving clarity and reducing complexity.
174-179: Updated time retrieval method inUpdateVersion.The change from
pstd::NowMillis()torocksdb::Env::Default()->GetCurrentTime(&unix_time)inUpdateVersionis correctly implemented. This ensures that the versioning logic is based on a more precise time source.src/pika_cache_load_thread.cc (1)
116-122: Simplification of TTL handling looks good.The renaming of
ttl_millsectottland the removal of conversion from milliseconds to seconds simplifies the code. Ensure that thettlis correctly handled elsewhere in the codebase.Verification successful
TTL Handling is Consistent Across the Codebase
The usage of
ttlis consistent throughout the codebase, with no conversions from milliseconds to seconds observed. This aligns with the simplification mentioned in the review comment.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ttl` in the codebase to ensure consistency. # Test: Search for the usage of `ttl` in the codebase. Expect: Consistent usage without conversion. rg --type cc --type cxx --type cpp -A 5 $'ttl'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage of `ttl` in the codebase to ensure consistency. # Test: Search for the usage of `ttl` in the codebase. Expect: Consistent usage without conversion. rg --type cpp --type cxx --type c -A 5 $'ttl'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage of `ttl` in the codebase to ensure consistency. # Test: Search for the usage of `ttl` in the codebase. Expect: Consistent usage without conversion. rg --type cpp --type c -A 5 $'ttl'Length of output: 140166
src/storage/src/lists_meta_value_format.h (1)
47-49: Timestamp handling simplification is beneficial.The removal of conversion logic for timestamps and the use of
rocksdb::Env::Default()->GetCurrentTimeimproves code readability. Verify that the change in time retrieval method does not adversely affect the precision or functionality.Also applies to: 54-55
src/storage/tests/lists_filter_test.cc (1)
105-105: Method renaming improves clarity.Renaming
SetRelativeTimeByMillsectoSetRelativeTimestampclarifies the method's purpose. Ensure that the method's functionality is consistent with its new name.Also applies to: 122-122
Verification successful
Method
SetRelativeTimestampusage is consistent with its intended functionality.The method
SetRelativeTimestampis used across multiple files, and its implementation correctly sets a timestamp relative to the current time. This aligns well with the method's purpose and new name. No further action is needed.
src/storage/src/base_value_format.h: DefinesSetRelativeTimestampto set a timestamp based on TTL.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `SetRelativeTimestamp` to ensure consistency with its new purpose. # Test: Search for all usages of `SetRelativeTimestamp` to ensure it aligns with its intended functionality. rg --type cc --type cxx --type cpp -A 5 $'SetRelativeTimestamp'Length of output: 89
Script:
#!/bin/bash # Description: Verify the usage of `SetRelativeTimestamp` to ensure consistency with its new purpose. # Test: Search for all usages of `SetRelativeTimestamp` to ensure it aligns with its intended functionality. rg --type cpp -A 5 $'SetRelativeTimestamp'Length of output: 8691
src/storage/src/base_filter.h (2)
31-33: Switch torocksdb::Env::Default()->GetCurrentTimefor time retrieval.The change from
pstd::NowMillis()torocksdb::Env::Default()->GetCurrentTime(&unix_time)standardizes time retrieval using RocksDB's environment. Ensure that this change aligns with the intended time granularity and correctness in the filtering logic.Verification successful
Consistent Usage of
rocksdb::Env::Default()->GetCurrentTimeVerifiedThe use of
rocksdb::Env::Default()->GetCurrentTimeis consistent across the codebase, indicating alignment with the intended time granularity and correctness in the filtering logic. No further action is required.
- Verified usage in multiple files including
src/storage/src/base_filter.h,src/storage/src/redis_sets.cc, and others.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `rocksdb::Env::Default()->GetCurrentTime` in the codebase. # Test: Search for the usage of `GetCurrentTime`. Expect: Consistent usage across the codebase. rg --type cpp -A 5 $'GetCurrentTime'Length of output: 15740
184-186: Consistent time retrieval inBaseDataFilter.Similar to
BaseMetaFilter, the time retrieval method has been updated to userocksdb::Env::Default()->GetCurrentTime(&unix_time). This ensures consistency across filters. Verify that the new method is correctly integrated and that any dependencies onpstd::NowMillis()are updated.Verification successful
Time retrieval method integration verified.
The integration of
rocksdb::Env::Default()->GetCurrentTimeis consistent across the codebase, including inBaseDataFilter. There are no remaining dependencies onpstd::NowMillis(), indicating that all necessary updates have been made.
- Instances of
GetCurrentTimefound in multiple files, confirming consistent usage.- No instances of
pstd::NowMillis()found, suggesting complete removal or replacement.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `rocksdb::Env::Default()->GetCurrentTime` in `BaseDataFilter`. # Test: Search for the usage of `GetCurrentTime` in `BaseDataFilter`. Expect: Consistent usage across the codebase. rg --type cpp -A 5 $'BaseDataFilter'Length of output: 4565
Script:
#!/bin/bash # Description: Check for the usage of `rocksdb::Env::Default()->GetCurrentTime` and `pstd::NowMillis()` in the codebase. # Search for the usage of `GetCurrentTime` in the codebase. rg --type cpp 'rocksdb::Env::Default()->GetCurrentTime' -A 3 # Search for the usage of `pstd::NowMillis()` in the codebase. rg --type cpp 'pstd::NowMillis()' -A 3Length of output: 103
Script:
#!/bin/bash # Description: Re-check for the usage of `rocksdb::Env::Default()->GetCurrentTime` and `pstd::NowMillis()` with broader patterns. # Search for the usage of `GetCurrentTime` in the codebase, considering possible variations. rg --type cpp 'GetCurrentTime' -A 3 # Search for the usage of `NowMillis` in the codebase, considering possible variations. rg --type cpp 'NowMillis' -A 3Length of output: 10481
include/pika_cache.h (1)
70-70: Parameter name change for clarity.The parameter name in the
Expireatmethod has been changed fromttl_sectottl. This change improves consistency with other methods in the class. Ensure that all calls toExpireatare updated to use the new parameter name.Verification successful
Parameter name change verified successfully.
The parameter name in the
Expireatmethod has been consistently updated tottlacross the codebase. No issues were found with the usage of this method.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `Expireat` method with the updated parameter name. # Test: Search for the usage of `Expireat` in the codebase. Expect: Consistent usage of the new parameter name. rg --type cpp -A 5 $'Expireat'Length of output: 14301
Script:
#!/bin/bash # Description: Verify the parameter name in the definition and calls of the `Expireat` method. # Search for the `Expireat` method definition to check the parameter name. ast-grep --lang cpp --pattern 'rocksdb::Status Expireat(std::string& key, int64_t $_)' # Search for calls to the `Expireat` method to verify the parameter name usage. rg --type cpp 'Expireat(' -A 2Length of output: 190
Script:
#!/bin/bash # Description: Verify the parameter name in the definition and calls of the `Expireat` method. # Search for the `Expireat` method definition to check the parameter name. ast-grep --lang cpp --pattern 'rocksdb::Status Expireat(std::string& key, int64_t $_)' # Correct the regex syntax and search for calls to the `Expireat` method to verify the parameter name usage. rg --type cpp 'Expireat\(' -A 2Length of output: 6438
src/pstd/src/env.cc (1)
220-222: UpdateNowMicrosto returnuint64_t.The return type of
NowMicroshas been changed touint64_t, which standardizes the time representation. Ensure that this change is compatible with the rest of the codebase and that any dependencies on the old return type are updated.Verification successful
Return Type Update for
NowMicrosVerifiedThe change of the
NowMicrosfunction's return type touint64_tis compatible with its usages throughout the codebase. All instances whereNowMicrosis used are consistent with this return type, ensuring no type mismatches or related issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `NowMicros` with the updated return type. # Test: Search for the usage of `NowMicros` in the codebase. Expect: Consistent usage of the new return type. rg --type cpp -A 5 $'NowMicros'Length of output: 26658
include/pika_kv.h (10)
39-44: Consistent Naming:sec_inSetCmd.The variable
ttl_millsechas been renamed tosec_, improving clarity and consistency.
72-72: Consistent Naming:sec_inGetCmd.The variable
ttl_millsec_has been renamed tosec_, which improves consistency.
354-354: Consistent Naming:sec_inSetexCmd.The variable
ttl_sec_has been renamed tosec_, aligning with the naming standardization.
379-379: Consistent Naming:usec_inPsetexCmd.The variable
ttl_millsechas been renamed tousec_, reflecting a more precise unit.
543-543: Consistent Naming:sec_inStrlenCmd.The variable
ttl_millsechas been renamed tosec_, aligning with the naming standardization.
584-584: Consistent Naming:sec_inExpireCmd.The variable
ttl_sec_has been renamed tosec_, aligning with the naming standardization.
608-608: Consistent Naming:msec_inPexpireCmd.The variable
ttl_millsechas been renamed tomsec_, improving clarity.
632-632: Consistent Naming:time_stamp_inExpireatCmd.The variable
time_stamp_sec_has been renamed totime_stamp_, simplifying the name.
655-658: Consistent Naming:time_stamp_ms_inPexpireatCmd.The variable
time_stamp_millsec_has been renamed totime_stamp_ms_, clarifying the unit.
813-815: Consistent Naming:time_stamp_inPKSetexAtCmd.The variable
time_stamp_sec_has been renamed totime_stamp_, simplifying the name.src/storage/src/redis.h (28)
120-120: Consistent Naming:ttlinStringsExpire.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
121-121: Consistent Naming:ttlinHashesExpire.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
122-122: Consistent Naming:ttlinListsExpire.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
123-123: Consistent Naming:ttlinZsetsExpire.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
124-124: Consistent Naming:ttlinSetsExpire.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
133-133: Consistent Naming:timestampinStringsExpireat.The parameter
timestamp_millsechas been renamed totimestamp, improving clarity and consistency.
134-134: Consistent Naming:timestampinHashesExpireat.The parameter
timestamp_millsechas been renamed totimestamp, improving clarity and consistency.
135-135: Consistent Naming:timestampinListsExpireat.The parameter
timestamp_millsechas been renamed totimestamp, improving clarity and consistency.
136-136: Consistent Naming:timestampinSetsExpireat.The parameter
timestamp_millsechas been renamed totimestamp, improving clarity and consistency.
137-137: Consistent Naming:timestampinZsetsExpireat.The parameter
timestamp_millsechas been renamed totimestamp, improving clarity and consistency.
145-145: Consistent Naming:timestampinStringsTTL.The parameter
ttl_millsechas been renamed totimestamp, improving clarity and consistency.
146-146: Consistent Naming:timestampinHashesTTL.The parameter
ttl_millsechas been renamed totimestamp, improving clarity and consistency.
147-147: Consistent Naming:timestampinListsTTL.The parameter
ttl_millsechas been renamed totimestamp, improving clarity and consistency.
148-148: Consistent Naming:timestampinZsetsTTL.The parameter
ttl_millsechas been renamed totimestamp, improving clarity and consistency.
149-149: Consistent Naming:timestampinSetsTTL.The parameter
ttl_millsechas been renamed totimestamp, improving clarity and consistency.
159-159: Consistent Naming:ttlinGetWithTTL.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
160-160: Consistent Naming:ttlinMGetWithTTL.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
164-164: Consistent Naming:ttlinGetrangeWithValue.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
172-172: Consistent Naming:ttlinSetxx.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
174-174: Consistent Naming:ttlinSetex.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
175-175: Consistent Naming:ttlinSetnx.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
176-176: Consistent Naming:ttlinSetvx.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
184-184: Consistent Naming:timestampinPKSetexAt.The parameter
time_stamp_millsec_has been renamed totimestamp, improving clarity and consistency.
188-188: Consistent Naming:timestampinExpire.The parameter
ttl_millsechas been renamed totimestamp, improving clarity and consistency.
189-189: Consistent Naming:timestampinExpireat.The parameter
timestamp_millsechas been renamed totimestamp, improving clarity and consistency.
191-191: Consistent Naming:timestampinTTL.The parameter
ttl_millsechas been renamed totimestamp, improving clarity and consistency.
201-201: Consistent Naming:ttlinHGetallWithTTL.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.
258-258: Consistent Naming:ttlinSMembersWithTTL.The parameter
ttl_millsechas been renamed tottl, improving clarity and consistency.src/storage/tests/strings_test.cc (4)
90-90: Simplified Time Representation:ExpireinAppendTest.The
Expiremethod now uses seconds instead of milliseconds, simplifying the test case.
767-767: Simplified Time Representation:ExpireinSetvxTest.The
Expiremethod now uses seconds instead of milliseconds, simplifying the test case.
944-946: Conditional Compilation for macOS inPKSetexAtTest.The test is skipped on macOS, which is appropriate for handling platform-specific differences.
Line range hint
953-985: Simplified Time Representation:PKSetexAtinPKSetexAtTest.The
PKSetexAtmethod now uses seconds instead of milliseconds, simplifying the test case.src/storage/include/storage/storage.h (15)
193-193: Verify method signature update forSetxx.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toSetxxare updated to use the new parameter name and unit (seconds).Verification successful
Verification Successful:
Setxxmethod signature updateThe
Setxxmethod has been updated to use thettlparameter in seconds, and this change is consistently reflected across the codebase. No further action is required.
- Locations where
Setxxis used withttlin seconds:
src/pika_kv.ccsrc/storage/src/storage.ccsrc/pika_cache.ccsrc/cache/src/string.ccScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `Setxx` use the updated `ttl` parameter. # Test: Search for `Setxx` usage. Expect: Use of `ttl` in seconds. rg --type cpp --type c++ --type c 'Setxx'Length of output: 69
Script:
#!/bin/bash # Description: Verify all calls to `Setxx` use the updated `ttl` parameter. # Corrected Test: Search for `Setxx` usage. Expect: Use of `ttl` in seconds. rg --type cpp 'Setxx'Length of output: 2676
201-201: Verify method signature update forGetWithTTL.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toGetWithTTLare updated to use the new parameter name and unit (seconds).
230-230: Verify method signature update forSetnx.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toSetnxare updated to use the new parameter name and unit (seconds).
296-296: Verify method signature update forSetex.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toSetexare updated to use the new parameter name and unit (seconds).Verification successful
Verification Successful:
SetexMethod Calls UpdatedAll calls to the
Setexmethod are using the updatedttlparameter in seconds. The changes are consistent across the codebase, ensuring correct usage of the TTL in seconds.
src/pika_kv.cc: Calls toSetexusesec_, indicating seconds.src/storage/tests/strings_test.cc: Calls use integer values directly in seconds.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `Setex` use the updated `ttl` parameter. # Test: Search for `Setex` usage. Expect: Use of `ttl` in seconds. rg --type cpp --type c++ --type c 'Setex'Length of output: 69
Script:
#!/bin/bash # Description: Verify all calls to `Setex` use the updated `ttl` parameter. # Test: Search for `Setex` usage. Expect: Use of `ttl` in seconds. rg --type cpp 'Setex'Length of output: 4621
963-963: Verify method signature update forExpire.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toExpireare updated to use the new parameter name and unit (seconds).Verification successful
Method signature update for
Expireverified.The
Expiremethod is being called with thettlparameter in seconds, as indicated by conversions from milliseconds to seconds in the codebase. This confirms the method signature update has been correctly applied.
- Instances in
src/pika_kv.ccshow conversions to seconds, ensuring compliance with the updated signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `Expire` use the updated `ttl` parameter. # Test: Search for `Expire` usage. Expect: Use of `ttl` in seconds. rg --type cpp --type c++ --type c 'Expire'Length of output: 70
Script:
#!/bin/bash # Description: Verify all calls to `Expire` use the updated `ttl` parameter. # Corrected file type and search for `Expire` usage. Expect: Use of `ttl` in seconds. rg --type cpp 'Expire'Length of output: 14112
470-470: Verify method signature update forSMembersWithTTL.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toSMembersWithTTLare updated to use the new parameter name and unit (seconds).
306-306: Verify method signature update forPKSetexAt.The timestamp parameter has been renamed from
time_stamp_millsec_totimestamp. Ensure that all calls toPKSetexAtare updated to use the new parameter name and unit (seconds).
241-241: Verify method signature update forSetvx.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toSetvxare updated to use the new parameter name and unit (seconds).
1013-1013: Verify method signature update forExpireat.The timestamp parameter has been renamed from
timestamp_millsectotimestamp. Ensure that all calls toExpireatare updated to use the new parameter name and unit (seconds).Verification successful
Method Signature Update Verified for
Expireat.The
Expireatmethod signature update to usetimestampin seconds has been correctly reflected across the codebase. Instances where conversion from milliseconds to seconds was necessary have been appropriately handled.
- Conversion from milliseconds to seconds is evident in
src/pika_kv.cc.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `Expireat` use the updated `timestamp` parameter. # Test: Search for `Expireat` usage. Expect: Use of `timestamp` in seconds. rg --type cpp --type c++ --type c 'Expireat'Length of output: 72
Script:
#!/bin/bash # Description: Verify all calls to `Expireat` use the updated `timestamp` parameter. # Test: Search for `Expireat` usage. Expect: Use of `timestamp` in seconds. rg --type cpp 'Expireat'Length of output: 4432
706-706: Verify method signature update forZRangeWithTTL.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toZRangeWithTTLare updated to use the new parameter name and unit (seconds).Verification successful
Method signature update for
ZRangeWithTTLverified.The parameter
ttl_millsechas been successfully renamed tottl, and no instances of the old parameter name were found in the codebase. All calls toZRangeWithTTLappear to use the updated parameter name.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `ZRangeWithTTL` use the updated `ttl` parameter. # Test: Search for `ZRangeWithTTL` usage. Expect: Use of `ttl` in seconds. rg --type cpp --type c++ --type c 'ZRangeWithTTL'Length of output: 77
Script:
#!/bin/bash # Description: Verify all calls to `ZRangeWithTTL` use the updated `ttl` parameter. # Test: Search for `ZRangeWithTTL` usage. Expect: Use of `ttl` in seconds. rg --type cpp 'ZRangeWithTTL'Length of output: 849
Script:
#!/bin/bash # Description: Search for any instances of the old parameter name `ttl_millsec`. # Test: Ensure `ttl_millsec` is no longer used in the codebase. rg 'ttl_millsec'Length of output: 16
258-258: Verify method signature update forGetrangeWithValue.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toGetrangeWithValueare updated to use the new parameter name and unit (seconds).Verification successful
Verification successful for
GetrangeWithValuemethod signature update.All calls to
GetrangeWithValuecorrectly use thettlparameter in seconds, confirming the update fromttl_millsectottlhas been applied consistently.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `GetrangeWithValue` use the updated `ttl` parameter. # Test: Search for `GetrangeWithValue` usage. Expect: Use of `ttl` in seconds. rg --type cpp --type c++ --type c 'GetrangeWithValue'Length of output: 81
Script:
#!/bin/bash # Description: Verify all calls to `GetrangeWithValue` use the updated `ttl` parameter. # Test: Search for `GetrangeWithValue` usage. Expect: Use of `ttl` in seconds. rg --type cpp 'GetrangeWithValue'Length of output: 726
Script:
#!/bin/bash # Description: Verify the unit of `sec_` in `src/pika_kv.cc`. # Test: Search for `sec_` definition or initialization. rg --type cpp 'sec_' src/pika_kv.cc -A 3 -B 3Length of output: 6927
108-109: Ensure consistency in TTL representation.The
ttl_millsecmember has been renamed tottl, indicating a change from milliseconds to seconds. Verify that all usages ofValueStatusacross the codebase reflect this change in time unit.Verification successful
TTL Representation Consistency Verified
The
ttlfield in theValueStatusstruct is consistently used as a time-to-live value in seconds across the codebase. The transition fromttl_millsectottlhas been correctly implemented and verified in various operations such asSet,Expire, andGetWithTTL.
- Verified consistent usage of
ttlin seconds across multiple files and functions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `ValueStatus` reflect the change in TTL representation from milliseconds to seconds. # Test: Search for `ValueStatus` usage. Expect: Consistent use of `ttl` in seconds. rg --type cpp --type c++ --type c 'ValueStatus'Length of output: 75
Script:
#!/bin/bash # Description: Verify all usages of `ValueStatus` reflect the change in TTL representation from milliseconds to seconds. # Test: Search for `ValueStatus` usage. Expect: Consistent use of `ttl` in seconds. rg --type cpp --type c 'ValueStatus'Length of output: 2901
Script:
#!/bin/bash # Description: Verify specific usages of the `ttl` field in `ValueStatus` to ensure it is consistently treated as seconds. # Test: Search for `ttl` usage in the context of `ValueStatus`. Expect: Consistent use as seconds. rg --type cpp --type c 'ttl' -A 3Length of output: 105553
1118-1118: Verify method signature update forMGetWithTTL.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toMGetWithTTLare updated to use the new parameter name and unit (seconds).Verification successful
Method Signature Update Verified for
MGetWithTTL.The method signature for
MGetWithTTLwith the updatedttlparameter is consistently used across the codebase. There are no instances of the old parameter namettl_millsec. The update appears to be correctly implemented.
src/pika_kv.ccsrc/storage/src/storage.ccsrc/storage/src/redis_strings.ccsrc/storage/include/storage/storage.hScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `MGetWithTTL` use the updated `ttl` parameter. # Test: Search for `MGetWithTTL` usage. Expect: Use of `ttl` in seconds. rg --type cpp --type c++ --type c 'MGetWithTTL'Length of output: 75
Script:
#!/bin/bash # Description: Verify all calls to `MGetWithTTL` use the updated `ttl` parameter. # Test: Search for `MGetWithTTL` usage. Expect: Use of `ttl` in seconds. rg --type cpp 'MGetWithTTL'Length of output: 953
Script:
#!/bin/bash # Extract and review lines around each usage of `MGetWithTTL` to verify parameter usage. rg --type cpp 'MGetWithTTL' -A 3 -B 3Length of output: 3910
337-337: Verify method signature update forHGetallWithTTL.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toHGetallWithTTLare updated to use the new parameter name and unit (seconds).
543-543: Verify method signature update forLRangeWithTTL.The TTL parameter has been renamed from
ttl_millsectottl. Ensure that all calls toLRangeWithTTLare updated to use the new parameter name and unit (seconds).src/pika_kv.cc (11)
Line range hint
25-55:
LGTM! Conversion from milliseconds to seconds is correct.The change from milliseconds to seconds for the
sec_variable is correctly implemented by dividing by 1000.
70-79: LGTM! Consistent use of second-based time representation.The use of
sec_in storage function calls is consistent with the new second-based time representation.
114-114: LGTM! Cache update with second-based TTL is correct.The use of
sec_for TTL in cache updates is consistent with the new second-based time representation.
136-136: LGTM! Timestamp calculation using seconds is correct.The calculation of the timestamp using
sec_is consistent with the new second-based time representation.
159-159: LGTM! Retrieval with second-based TTL is correct.The use of
sec_for storing TTL is consistent with the new second-based time representation.
189-189: LGTM! Cache update with second-based TTL is correct.The use of
sec_for TTL in cache updates is consistent with the new second-based time representation.
572-572: No changes or issues to review inAppendCmd::DoThroughDB.
635-635: LGTM! Retrieval with TTL for multiple keys is correct.The function correctly handles retrieval with TTL, consistent with the new second-based time representation.
707-707: LGTM! Cache update with TTL for multiple keys is correct.The use of TTL in cache updates for multiple keys is consistent with the new second-based time representation.
884-884: LGTM! Timestamp calculation using seconds is correct.The use of
expired_timestamp_sec_for timestamp calculation is consistent with the new second-based time representation.
1468-1468: LGTM! Conversion from milliseconds to seconds is correct.The conversion from milliseconds to seconds for the timestamp is correctly handled.
src/storage/src/redis_lists.cc (6)
33-34: Verify the usage ofGetCurrentTime.Ensure that
rocksdb::Env::Default()->GetCurrentTime(&curtime)is correctly used and returns the expected time format. This change might affect time accuracy and consistency.Verification successful
Consistent Usage of
GetCurrentTimeVerifiedThe usage of
rocksdb::Env::Default()->GetCurrentTimeis consistent across various files in the codebase. It is used in different contexts, such as calculating time-to-live (TTL) and timestamps, and appears to be implemented correctly without any evident issues.
src/storage/src/redis_lists.ccsrc/storage/src/redis_sets.ccsrc/storage/src/redis_strings.ccsrc/storage/src/redis_zsets.ccsrc/storage/src/redis_hashes.ccsrc/storage/src/zsets_filter.hsrc/storage/src/strings_filter.hsrc/storage/src/lists_meta_value_format.hsrc/storage/src/base_filter.hsrc/storage/src/base_value_format.hsrc/storage/src/base_meta_value_format.htools/pika-port/pika_port_3/migrator_thread.ccThe change should not affect time accuracy or consistency adversely. No further action is needed unless specific issues are observed during runtime testing.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `GetCurrentTime` across the codebase. # Test: Search for the usage of `GetCurrentTime`. Expect: Consistent usage across the codebase. rg --type cc -A 2 'GetCurrentTime'Length of output: 61
Script:
#!/bin/bash # Description: Verify the usage of `GetCurrentTime` across the codebase. # Test: Search for the usage of `GetCurrentTime`. Expect: Consistent usage across the codebase. rg --type cpp -A 2 'GetCurrentTime'Length of output: 8445
Line range hint
1177-1206: Verify consistent usage oftimestamp.Ensure that the parameter
timestampis used consistently and correctly throughout the function.Verification successful
Consistent Usage of
timestampVerifiedThe parameter
timestampis used consistently in both the definition and declaration of theListsExpireatfunction. The related call inredis_strings.ccuses a parameterttl, which aligns withtimestamp. No inconsistencies were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `timestamp` in `ListsExpireat`. # Test: Search for the usage of `ListsExpireat` to ensure consistent handling of `timestamp`. rg --type cc -A 5 'ListsExpireat'Length of output: 60
Script:
#!/bin/bash # Description: Verify consistent usage of `timestamp` in `ListsExpireat`. # Test: Search for the usage of `ListsExpireat` to ensure consistent handling of `timestamp`. rg --type cpp -A 5 'ListsExpireat'Length of output: 1502
Line range hint
474-508: Verify TTL calculation logic and time retrieval.Ensure the logic for calculating TTL using
ttlis correct and verify the impact of usingrocksdb::Env::Default()->GetCurrentTime(&curtime).Verification successful
TTL Calculation Logic and Time Retrieval Verified
The
LRangeWithTTLfunction's TTL calculation logic and its usage ofrocksdb::Env::Default()->GetCurrentTime(&curtime)appear to be correctly implemented and utilized across the codebase. No issues were found with the current implementation.
- Usage in
pika_cache_load_thread.ccconfirms that TTL values are being retrieved and used appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TTL calculation logic and time retrieval. # Test: Search for the usage of `LRangeWithTTL` to ensure correct handling of `ttl`. rg --type cc -A 5 'LRangeWithTTL'Length of output: 60
Script:
#!/bin/bash # Description: Verify TTL calculation logic and time retrieval. # Test: Search for the usage of `LRangeWithTTL` to ensure correct handling of `ttl`. rg --type cpp -A 5 'LRangeWithTTL'Length of output: 2713
Line range hint
1256-1296: Verify consistent usage oftimestamp.Ensure that the parameter
timestampis used consistently and correctly throughout the function.Verification successful
Consistent Usage of
timestampVerifiedThe parameter
timestampis used consistently and correctly throughout theListsTTLfunction and its related declarations and invocations. No issues were found regarding its usage.
ListsTTLfunction definition insrc/storage/src/redis_lists.ccListsTTLfunction declaration insrc/storage/src/redis.h- Invocation in
src/storage/src/redis_strings.ccScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `timestamp` in `ListsTTL`. # Test: Search for the usage of `ListsTTL` to ensure consistent handling of `timestamp`. rg --type cc -A 5 'ListsTTL'Length of output: 55
Script:
#!/bin/bash # Description: Verify consistent usage of `timestamp` in `ListsTTL`. # Test: Search for the usage of `ListsTTL` to ensure consistent handling of `timestamp`. rg --type cpp -A 5 'ListsTTL'Length of output: 1485
Line range hint
1100-1130: Verify consistent usage ofttl.Ensure that the parameter
ttlis used consistently and correctly throughout the function.Verification successful
Consistent usage of
ttlverified.The parameter
ttlis used consistently and correctly in theListsExpirefunction across the codebase. There are no issues with the parameter renaming.
ListsExpireis correctly declared insrc/storage/src/redis.h.ListsExpireis correctly implemented insrc/storage/src/redis_lists.cc.ListsExpireis correctly called insrc/storage/src/redis_strings.cc.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of `ttl` in `ListsExpire`. # Test: Search for the usage of `ListsExpire` to ensure consistent handling of `ttl`. rg --type cc -A 5 'ListsExpire'Length of output: 58
Script:
#!/bin/bash # Description: Verify consistent usage of `ttl` in `ListsExpire`. # Test: Search for the usage of `ListsExpire` to ensure consistent handling of `ttl`. rg --type cpp -A 5 'ListsExpire'Length of output: 2927
Line range hint
1330-1330: Verify time retrieval method consistency.Ensure that the time retrieval method used in
ScanListsis consistent with other parts of the code.Verification successful
Time Retrieval Method in
ScanListsis ConsistentThe
ScanListsfunction inredis_lists.ccusestime(nullptr)to retrieve the current time, which is a standard and consistent method for obtaining the current time in C++. This approach aligns with typical practices and should be consistent with other parts of the codebase unless a specific alternative method is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify time retrieval method consistency in `ScanLists`. # Test: Search for the usage of `ScanLists` and ensure consistent time retrieval methods. rg --type cc -A 5 'ScanLists'Length of output: 56
Script:
#!/bin/bash # Description: Verify time retrieval method consistency in `ScanLists`. # Test: Search for the usage of `ScanLists` in the codebase. rg -A 5 'ScanLists'Length of output: 2588
Script:
#!/bin/bash # Description: Extract the implementation of `ScanLists` to verify the time retrieval method. # Extract the implementation of `ScanLists` from `redis_lists.cc`. ast-grep --lang cpp --pattern 'void ScanLists() { $$$ }'Length of output: 58
Script:
#!/bin/bash # Description: Search for the implementation of `ScanLists` in `redis_lists.cc` with more context. # Search for `ScanLists` in `redis_lists.cc` and include more lines of context. rg 'void ScanLists' -A 20 --type cppLength of output: 1385
Script:
#!/bin/bash # Description: Perform a broader search for the implementation of `ScanLists` in `redis_lists.cc`. # Search for `ScanLists` in `redis_lists.cc` and include a larger number of context lines. rg 'ScanLists' -A 50 --type cppLength of output: 19501
src/storage/src/redis_hashes.cc (10)
34-35: Good use of RocksDB's environment for time retrieval.Replacing the custom time function with
rocksdb::Env::Default()->GetCurrentTime(&curtime)improves consistency and potentially enhances performance.
119-119: Ensure data integrity with count checks.The check for modifying the count before decrementing ensures that the count does not become negative, maintaining data integrity.
Line range hint
224-258:
Clear and correct TTL handling inHGetallWithTTL.The logic for calculating TTL is clear and handles edge cases like permanent survival and expired entries effectively.
335-335: Ensure data integrity with count checks.The check for modifying the count before incrementing ensures that the count does not exceed its limits, maintaining data integrity.
429-429: Ensure data integrity with count checks.The check for modifying the count before incrementing ensures that the count does not exceed its limits, maintaining data integrity.
657-657: Ensure data integrity with count checks.The check for modifying the count before incrementing ensures that the count does not exceed its limits, maintaining data integrity.
725-725: Ensure data integrity with count checks.The check for modifying the count before incrementing ensures that the count does not exceed its limits, maintaining data integrity.
792-792: Ensure data integrity with count checks.The check for modifying the count before incrementing ensures that the count does not exceed its limits, maintaining data integrity.
Line range hint
1160-1355:
Improved clarity and consistency in TTL handling.The changes in
HashesExpire,HashesExpireat, andHashesTTLimprove clarity and consistency in handling TTL values. The logic for setting and calculating TTL appears correct.
1125-1125: Verify the impact of changingstart_key_versiontype.The type of
start_key_versionhas been changed fromuint64_ttoint32_t. Ensure that this change does not introduce any overflow or underflow issues.src/pika_cache.cc (1)
129-132: Parameter name change approved.The change from
ttl_sectottlin theExpireatmethod improves consistency with other methods in the class.Ensure that all calls to
Expireatacross the codebase are updated to use the new parameter name.Verification successful
Parameter name change successfully propagated.
The change from
ttl_sectottlin theExpireatmethod has been consistently applied across the codebase. All relevant calls to the method now use the new parameter name.
- No instances of the old parameter name were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `Expireat` use the new parameter name `ttl`. # Test: Search for the function usage. Expect: Only occurrences of the new parameter name. rg --type cpp -A 5 $'Expireat'Length of output: 14301
src/storage/src/redis_strings.cc (14)
36-37: Consistent time handling.The use of
int64_tforcurtimeis consistent with the standardized time handling changes across the file.
385-386: Consistent time handling.The use of
int64_tforcurrent_timealigns with the standardized time handling changes.
390-397: Centralized TTL calculation.Using
CalculateTTLfor TTL calculations centralizes logic and enhances maintainability.
Line range hint
402-423: Promotes code reuse.Utilizing
HandleParsedStringsValueenhances code reuse and consistency in handling parsed string values.
Line range hint
429-443: Promotes code reuse.The use of
HandleParsedStringsValueinMGetWithTTLensures consistent handling of parsed string values.
Line range hint
784-817: Consistent time handling.The use of
int64_tforttlinSetxxaligns with the standardized approach to time handling.
882-887: Consistent time handling.The use of
int64_tforttlinSetexis consistent with the overall time handling changes.
Line range hint
897-915: Consistent time handling.The use of
int64_tforttlinSetnxaligns with the standardized approach to time handling.
Line range hint
925-950: Consistent time handling.The use of
int64_tforttlinSetvxis consistent with the overall time handling changes.
1295-1300: Consistent time handling.The use of
int64_tfortimestampinPKSetexAtaligns with the standardized approach to time handling.
Line range hint
1304-1331: Consistent time handling.The use of
int64_tforttlinStringsExpireis consistent with the overall time handling changes.
Line range hint
1371-1398: Consistent time handling.The use of
int64_tfortimestampinStringsExpireataligns with the standardized approach to time handling.
1575-1591: Consistent time handling.The use of
int64_tforttlinExpireis consistent with the overall time handling changes.
1599-1615: Consistent time handling.The use of
int64_tforttlinExpireataligns with the standardized approach to time handling.src/storage/src/redis_sets.cc (5)
37-38: LGTM! Consistent time retrieval method.The use of
rocksdb::Env::Default()->GetCurrentTime(&curtime)is a consistent approach for time retrieval across the codebase.
Line range hint
746-780:
LGTM! Improved time handling and parameter naming.The use of
rocksdb::Env::Default()->GetCurrentTime(&curtime)and the renaming of the parameter tottlenhance consistency and clarity.
Line range hint
1410-1440:
LGTM! Consistent parameter naming for TTL.The change from
ttl_millsectottlimproves clarity and consistency in parameter naming.
Line range hint
1487-1516:
LGTM! Consistent parameter naming for timestamp.The change from
timestamp_millsectotimestampimproves clarity and consistency in parameter naming.
Line range hint
1566-1605:
LGTM! Consistent parameter naming for timestamp.The change from
ttl_millsectotimestampimproves clarity and consistency in parameter naming.src/storage/src/redis_zsets.cc (12)
39-40: Use of RocksDB for time retrieval is appropriate.The change to use
rocksdb::Env::Default()->GetCurrentTime(&curtime)enhances precision and consistency with RocksDB's environment. Ensure this change is consistently applied across the codebase.
111-111: Modification count check is correctly implemented.The check ensures that the zset size does not overflow, maintaining data integrity.
169-169: Modification count check is correctly implemented.This check ensures the zset size remains within valid limits, preventing overflow errors.
264-264: Modification count check is correctly implemented.The check ensures that the zset size does not overflow, maintaining data integrity.
447-447: Modification count check is correctly implemented.This ensures the zset size remains valid, preventing overflow errors.
800-800: Modification count check is correctly implemented.This check helps prevent zset size overflow, ensuring data integrity.
867-867: Modification count check is correctly implemented.This ensures the zset size remains within valid limits, preventing overflow errors.
947-947: Modification count check is correctly implemented.This ensures the zset size remains valid, preventing overflow errors.
1659-1659: Modification count check is correctly implemented.This check helps prevent zset size overflow, ensuring data integrity.
1750-1750: Parameter name standardization is appropriate.The change from
timestamp_millsectotimestampaligns with the effort to standardize parameter names. Ensure all references to this function are updated.Verification successful
Parameter name
timestampis consistently used across the codebase.The function
ZsetsExpireatis correctly using the updated parameter nametimestampin all relevant locations. No instances of the old parameter name were found.
src/storage/src/redis_strings.cc: Function call usestimestamp.src/storage/src/redis.h: Function declaration usestimestamp.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ZsetsExpireat` use the updated parameter name `timestamp`. # Test: Search for the function usage. Expect: Only occurrences of the updated signature. rg --type cpp -A 5 $'ZsetsExpireat'Length of output: 1519
1674-1674: Parameter name standardization is appropriate.The change from
ttl_millsectottlaligns with the effort to standardize parameter names. Ensure all references to this function are updated.Verification successful
Parameter name updated consistently across the codebase.
The change from
ttl_millsectottlin theZsetsExpirefunction has been applied consistently across its definition, declaration, and usage. No further updates are necessary.
src/storage/src/redis.h: Declaration usesttl.src/storage/src/redis_zsets.cc: Definition usesttl.src/storage/src/redis_strings.cc: Function call usesttl.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ZsetsExpire` use the updated parameter name `ttl`. # Test: Search for the function usage. Expect: Only occurrences of the updated signature. rg --type cpp -A 5 $'ZsetsExpire'Length of output: 2982
1915-1915: Parameter name standardization is appropriate.The change from
ttl_millsectotimestampaligns with the effort to standardize parameter names. Ensure all references to this function are updated.Verification successful
Parameter name standardization verified successfully.
The function
ZsetsTTLconsistently uses the updated parameter nametimestampacross its definition, declaration, and usage in the codebase. No further updates are needed.
src/storage/src/redis_zsets.cc: Function definition usestimestamp.src/storage/src/redis_strings.cc: Function call usestimestamp.src/storage/src/redis.h: Function declaration usestimestamp.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ZsetsTTL` use the updated parameter name `timestamp`. # Test: Search for the function usage. Expect: Only occurrences of the updated signature. rg --type cpp -A 5 $'ZsetsTTL'Length of output: 1553
src/storage/tests/keys_test.cc (1)
5182-5183: Verify the appropriateness of usingrocksdb::Env::Default()->GetCurrentTime.The change in the method of obtaining the current time may affect the precision or context in which the time is retrieved. Ensure that
rocksdb::Env::Default()->GetCurrentTimeprovides the necessary precision and is suitable for the test's requirements.
| uint64_t start_us = pstd::NowMicros(); | ||
|
|
There was a problem hiding this comment.
Consider using a consistent time retrieval method.
The use of pstd::NowMicros() is inconsistent with other parts of the code that use rocksdb::Env::Default()->GetCurrentTime(&curtime). Consider refactoring for consistency.
| rocksdb::Status Redis::TTL(const Slice& key, int64_t* timestamp) { | ||
| std::string meta_value; | ||
| BaseMetaKey base_meta_key(key); | ||
| rocksdb::Status s = db_->Get(default_read_options_, handles_[kMetaCF], base_meta_key.Encode(), &meta_value); | ||
| if (s.ok()) { | ||
| auto type = static_cast<DataType>(static_cast<uint8_t>(meta_value[0])); | ||
| switch (type) { | ||
| case DataType::kSets: | ||
| return SetsTTL(key, ttl_millsec, std::move(meta_value)); | ||
| return SetsTTL(key, timestamp, std::move(meta_value)); | ||
| case DataType::kZSets: | ||
| return ZsetsTTL(key, ttl_millsec, std::move(meta_value)); | ||
| return ZsetsTTL(key, timestamp, std::move(meta_value)); | ||
| case DataType::kHashes: | ||
| return HashesTTL(key, ttl_millsec, std::move(meta_value)); | ||
| return HashesTTL(key, timestamp, std::move(meta_value)); | ||
| case DataType::kLists: | ||
| return ListsTTL(key, ttl_millsec, std::move(meta_value)); | ||
| return ListsTTL(key, timestamp, std::move(meta_value)); | ||
| case DataType::kStrings: | ||
| return StringsTTL(key, ttl_millsec, std::move(meta_value)); | ||
| return StringsTTL(key, timestamp, std::move(meta_value)); |
There was a problem hiding this comment.
Refactor TTL calculation.
Consider using CalculateTTL to avoid repeating TTL calculation logic and enhance consistency.
- *timestamp = parsed_strings_value.Etime();
- if (*timestamp == 0) {
- *timestamp = -1;
- } else {
- int64_t curtime;
- rocksdb::Env::Default()->GetCurrentTime(&curtime);
- *timestamp = *timestamp - curtime >= 0 ? *timestamp - curtime : -2;
- }
+ *timestamp = CalculateTTL(parsed_strings_value.Etime());Committable suggestion was skipped due to low confidence.
luky116
changed the title
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)
- .github/workflows/pika.yml (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pika.yml
…clear space in CI after compile (OpenAtomFoundation#2853) * fix:remove dependence in CI * Revert "fix: change storage ttl time from seconds to milliseconds (OpenAtomFoundation#2822)" --------- Co-authored-by: cjh <1271435567@qq.com> Co-authored-by: chejinge <chejinge@360.cn>
Summary by CodeRabbit
ttl_millsectottl, enhancing clarity and consistency across various methods.rocksdb::Env::Default()->GetCurrentTime()for improved accuracy and consistency.