feat: Add Keyspace hits metrics for set #2579
Conversation
|
基本的验证
集成测试
|
AlexStocks
changed the title
|
@chenbt-hz please fix the confliction |
done |
|
@CodiumAI-Agent /review |
PR Review 🔍
Code feedback:
|
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
| @@ -963,7 +970,7 @@ bool Cmd::IsSuspend() const { return (flag_ & kCmdFlagsSuspend); } | |||
| // std::string Cmd::CurrentSubCommand() const { return ""; }; | |||
| bool Cmd::HasSubCommand() const { return subCmdName_.size() > 0; }; | |||
| std::vector<std::string> Cmd::SubCommand() const { return subCmdName_; }; | |||
| bool Cmd::IsAdminRequire() const { return (flag_ & kCmdFlagsAdminRequire); } | |||
| bool Cmd::IsAdmin() const { return (flag_ & kCmdFlagsAdmin); } | |||
There was a problem hiding this comment.
IsAdminRequire 表示的含义会比 IsAdmin 更合适，修改的原因是啥呢
There was a problem hiding this comment.
IsAdminRequire没有使用。而且原来的kCmdFlagsAdminRequire不符合来着。你觉得IsAdminRequire这个方法名更合适吗？
2.upgrade Do() functions in pika_set.cc
WalkthroughThis update introduces enhancements to the command handling and performance tracking of the server. Key changes include the addition of a new enumerator Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Cmd
Client->>Cmd: Execute Command
Cmd->>Server: Check Key Existence
alt Key Exists
Server-->>Cmd: Key Found
Cmd->>Client: Return Success Response
else Key Does Not Exist
Server-->>Cmd: Key Not Found
Cmd->>Server: Increment Keyspace Misses
Cmd->>Client: Return No Exists Response
end
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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 (9)
- include/pika_command.h (4 hunks)
- include/pika_server.h (1 hunks)
- include/pika_statistic.h (1 hunks)
- src/pika_admin.cc (1 hunks)
- src/pika_command.cc (2 hunks)
- src/pika_server.cc (1 hunks)
- src/pika_set.cc (11 hunks)
- tests/integration/integrate_test.sh (1 hunks)
- tests/integration/server_test.go (2 hunks)
Additional comments not posted (30)
tests/integration/integrate_test.sh (4)
8-9: Useful addition for debugging environment variables.Printing
PATHandGOBINhelps in debugging and ensuring the correct paths are set for Go binaries.
12-14: Necessary installations for testing framework.Installing Ginkgo and Gomega ensures the necessary tools are available for testing.
16-16: Useful dry-run for Ginkgo tests.Running a dry-run helps verify the tests without executing them, and filtering out timing information makes the output cleaner.
18-19: Increased timeout for extensive test scenarios.Increasing the timeout to 60 minutes accommodates more extensive test scenarios and prevents premature timeouts.
include/pika_statistic.h (1)
40-41: Enhanced monitoring with keyspace metrics.Adding
keyspace_hitsandkeyspace_missesas atomic variables enhances the server's performance monitoring capabilities.include/pika_server.h (2)
250-251: Methods for retrieving keyspace metrics.Adding
ServerKeyspaceHitsandServerKeyspaceMissesmethods is necessary for retrieving keyspace metrics.
254-255: Methods for incrementing keyspace metrics.Adding
incr_server_keyspace_hitsandincr_server_keyspace_missesmethods is necessary for updating keyspace metrics.src/pika_set.cc (10)
80-80: Verify the performance impact of frequent atomic operations.Ensure that the frequent atomic operations for incrementing keyspace hits and misses do not significantly impact performance. Consider benchmarking this change.
127-131: LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.
173-181: LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.
Line range hint
259-278:
LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.
299-302: LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.
333-333: LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.
500-502: LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.
601-607: LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.
Line range hint
677-690:
LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.
333-333: LGTM!The changes correctly handle the
IsNotFoundstatus separately, ensuring accurate response updates.include/pika_command.h (4)
335-336: LGTM!The addition of the
kNoExistsenumerator is consistent with the changes in the command methods to handle non-existent keys.
342-342: LGTM!The
noexistmethod provides a convenient way to check if the response state iskNoExists.
343-343: LGTM!The modification ensures that the
okmethod returns true for thekNoExistsstate, broadening the scenarios in which a command can be considered successful.
547-547: LGTM!The renaming of the
IsAdminRequiremethod toIsAdminlikely reflects a simplification or clarification of the method's purpose.tests/integration/server_test.go (2)
15-26: LGTM!The
extractKeyspaceHitsfunction correctly parses the info output and retrieves the value associated with the specified keyword.
465-502: LGTM!The
should Info keyspace hitstest case performs the necessary operations to trigger updates in the keyspace metrics and verifies the accuracy of the reported metrics.src/pika_command.cc (2)
992-992: Verify consistent usage of the renamed methodIsAdmin.The method
IsAdminRequirehas been renamed toIsAdmin. Ensure that this new name is used consistently throughout the codebase to avoid any potential issues.Verification successful
The method
IsAdminis used consistently throughout the codebase.The search results indicate that the method
IsAdminhas been correctly renamed and is used consistently in both the header and implementation files.
include/pika_command.h:bool IsAdmin() const;src/pika_command.cc:bool Cmd::IsAdmin() const { return (flag_ & kCmdFlagsAdmin); }src/pika_command.cc:if (!IsAdmin() && res().ok()) {Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of the renamed method `IsAdmin`. # Test: Search for the old method name. Expect: No occurrences of `IsAdminRequire`. rg --type cpp 'IsAdminRequire'Length of output: 30
Script:
#!/bin/bash # Description: Verify the usage of the renamed method `IsAdmin`. # Test: Search for occurrences of the new method name `IsAdmin`. rg --type cpp 'IsAdmin'Length of output: 207
912-918: Ensure accurate keyspace hit/miss tracking and benchmark performance impact.The logic for incrementing keyspace hits and misses based on the command result is correct. However, frequent atomic operations may impact performance. It is recommended to benchmark these changes to ensure they do not introduce significant overhead.
src/pika_server.cc (4)
989-989: LGTM!The method
ServerKeyspaceHitscorrectly returns the current count of keyspace hits.
990-990: LGTM!The method
ServerKeyspaceMissescorrectly returns the current count of keyspace misses.
993-993: LGTM!The method
incr_server_keyspace_hitscorrectly increments the keyspace hits counter using an atomic operation.
994-994: LGTM!The method
incr_server_keyspace_missescorrectly increments the keyspace misses counter using an atomic operation.src/pika_admin.cc (1)
1012-1013: LGTM! Ensure consistency with other metrics.The additions for
keyspace_hitsandkeyspace_missesare consistent with the existing metrics reporting.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
include/pika_command.h (2)
335-336: LGTM. Consider adding documentation for the new enum value.The addition of
kNoExiststo theCmdRetenum is consistent with the PR objectives. It provides a clear way to represent when a key does not exist.Consider adding a brief comment explaining the purpose of this new enum value, e.g.:
kNoExists, // Indicates that the requested key does not exist
435-436: Consider a more consistent approach for thekNoExistscase.The addition of the
kNoExistscase in themessage()method is consistent with the other changes. However, it differs from other error cases by directly returningmessage_instead of constructing a specific error message.For consistency and clarity, consider one of the following approaches:
Construct a specific error message for
kNoExists, similar to other cases:case kNoExists: return "-ERR key does not exist\r\n";If custom messages are intended for
kNoExists, consider adding a comment explaining this behavior:case kNoExists: // Custom message for non-existent keys return message_;Please clarify the intended behavior for
kNoExistsmessages and ensure it aligns with the overall error reporting strategy.tests/integration/server_test.go (1)
Line range hint
1-595: LGTM with minor suggestionsThe additions to this test file are valuable and enhance the coverage of keyspace hits and misses functionality. The new
extractKeyspaceHitsfunction and the "should Info keyspace hits" test case are well-integrated with the existing test structure.Consider adding a brief comment above the
extractKeyspaceHitsfunction to explain its purpose and expected input/output. This would improve the readability and maintainability of the test file.src/pika_command.cc (1)
913-919: LGTM! Consider a minor optimization.The implementation for tracking keyspace hits and misses looks good and aligns with the PR objectives. It correctly increments the appropriate counter based on the command result.
Consider combining the conditions to reduce nesting:
- if (!IsAdmin() && res().ok()) { - if (res().noexist()) { - g_pika_server->incr_server_keyspace_misses(); - } else { - g_pika_server->incr_server_keyspace_hits(); - } - } + if (!IsAdmin() && res().ok()) { + res().noexist() ? g_pika_server->incr_server_keyspace_misses() : g_pika_server->incr_server_keyspace_hits(); + }This change would make the code more concise without affecting its functionality.
src/pika_server.cc (1)
1005-1006: LGTM: New functions to increment keyspace hits and misses.The
incr_server_keyspace_hitsandincr_server_keyspace_missesfunctions are correctly implemented using atomic operations for thread-safe increments.For consistency with other similar functions in the class, consider adding a blank line between these two functions.
void PikaServer::incr_server_keyspace_hits() { ++(statistic_.server_stat.keyspace_hits); } + void PikaServer::incr_server_keyspace_misses() { ++(statistic_.server_stat.keyspace_misses); }include/pika_server.h (2)
252-253: Use consistent data types for performance countersThe methods
ServerKeyspaceHits()andServerKeyspaceMisses()returnlong long, whereas similar methods likeServerQueryNum()andServerCurrentQps()returnuint64_t. For consistency and to prevent potential issues with negative values, consider changing the return type touint64_t.Apply this diff to update the return types:
-long long ServerKeyspaceHits() const; -long long ServerKeyspaceMisses() const; +uint64_t ServerKeyspaceHits() const; +uint64_t ServerKeyspaceMisses() const;
256-257: Maintain consistent naming conventions for method identifiersThe methods
incr_server_keyspace_hits()andincr_server_keyspace_misses()use snake_case naming, while other methods in thePikaServerclass predominantly use camelCase, such asincrAccumulativeConnections(). For consistency and readability, consider renaming these methods to use camelCase.Apply this diff to update the method names:
-void incr_server_keyspace_hits(); -void incr_server_keyspace_misses(); +void incrServerKeyspaceHits(); +void incrServerKeyspaceMisses();src/pika_admin.cc (1)
1073-1074: LGTM: Correctly adding keyspace hit/miss statisticsThe addition of
keyspace_hitsandkeyspace_missesstatistics to theInfoStatsoutput enhances monitoring capabilities. The implementation is consistent with existing code patterns.Consider adding unit tests to validate the accuracy of these new statistics under various scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- include/pika_command.h (3 hunks)
- include/pika_server.h (1 hunks)
- src/pika_admin.cc (1 hunks)
- src/pika_command.cc (2 hunks)
- src/pika_server.cc (1 hunks)
- tests/integration/server_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (7)
include/pika_command.h (3)
342-342: LGTM. Thenoexist()method is a good addition.The
noexist()method is a logical complement to the newkNoExistsenum value. It follows the pattern of other similar methods in the class and provides a clear way to check for this specific state.
343-343: Please clarify the rationale for consideringkNoExistsas "ok".The modification of the
ok()method to includeret_ == kNoExistsin its condition is not immediately intuitive. This change implies that a non-existent key is considered a successful operation, which may lead to unexpected behavior in client code.Could you provide more context on why
kNoExistsshould be treated as "ok"? Depending on the use case, it might be more appropriate to keep these states separate. Consider alternatives such as:
- Leaving
ok()as is and creating a new method likeisValidResult()that includeskNoExists.- Renaming
ok()to something more general likeisExpectedResult()to reflect its broader meaning.Please clarify the intended behavior and its impact on existing code that relies on the
ok()method.
548-548: LGTM. Method renaming improves clarity.The renaming of
IsAdminRequire()toIsAdmin()is a good change that improves code readability. The new name is more concise and clearly expresses the method's purpose.Ensure that any related documentation or comments referencing this method are updated to reflect the new name. Also, verify that all calls to this method throughout the codebase have been updated.
To ensure all references have been updated, run the following script:
✅ Verification successful
Verification Successful: All References Updated
The search confirmed that there are no remaining references to
IsAdminRequirein the codebase. The method renaming toIsAdmin()has been successfully completed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to IsAdminRequire rg "IsAdminRequire" --type cppLength of output: 30
Script:
#!/bin/bash # Search for any remaining references to IsAdminRequire and handle potential errors rg "IsAdminRequire" --type cpp if [ $? -eq 0 ]; then echo "References to IsAdminRequire found." else echo "No references to IsAdminRequire found or an error occurred." fiLength of output: 177
src/pika_command.cc (1)
993-993: LGTM! Method simplification and renaming approved.The simplification of the
IsAdminmethod and its renaming fromIsAdminRequiretoIsAdminis a good improvement. It makes the API more intuitive and consistent with the flag-based approach used in the class.This change successfully addresses the previous discussions and feedback mentioned in the past review comments.
src/pika_server.cc (2)
1001-1002: LGTM: New function to retrieve keyspace hits.The
ServerKeyspaceHitsfunction is correctly implemented, using atomic load for thread-safe access to the keyspace hits statistic.
1002-1003: LGTM: New function to retrieve keyspace misses.The
ServerKeyspaceMissesfunction is correctly implemented, using atomic load for thread-safe access to the keyspace misses statistic.include/pika_server.h (1)
256-257:⚠️ Potential issueEnsure thread safety by using atomic variables for keyspace hit/miss counters
To prevent race conditions in the multi-threaded environment of
PikaServer, ensure that the variables updated byincr_server_keyspace_hits()andincr_server_keyspace_misses()are declared using atomic types, such asstd::atomic<uint64_t>.Run the following script to verify that the keyspace hit/miss counters are declared as atomic variables:
tests/integration/server_test.go
Outdated
| It("should Info keyspace hits", func() { | ||
| sRem := client.SRem(ctx, "keyspace_hits", "one") | ||
| Expect(sRem.Err()).NotTo(HaveOccurred()) | ||
| sAdd := client.SAdd(ctx, "keyspace_hits", "one") | ||
| Expect(sAdd.Err()).NotTo(HaveOccurred()) | ||
|
|
||
| info := client.Info(ctx, "stats") | ||
| Expect(info.Err()).NotTo(HaveOccurred()) | ||
| Expect(info.Val()).NotTo(Equal("")) | ||
| Expect(info.Val()).To(ContainSubstring("keyspace_hits")) | ||
| Expect(info.Val()).To(ContainSubstring("keyspace_misses")) | ||
| oldInfoKeyspaceHitsStr := extractKeyspaceHits(info.Val(), "keyspace_hits") | ||
| oldInfoKeyspaceHits, err := strconv.ParseInt(oldInfoKeyspaceHitsStr, 10, 64) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| oldInfoKeyspaceMissesStr := extractKeyspaceHits(info.Val(), "keyspace_misses") | ||
| oldInfoKeyspaceMisses, err := strconv.ParseInt(oldInfoKeyspaceMissesStr, 10, 64) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(client.SMembers(ctx, "keyspace_hits").Err()).NotTo(HaveOccurred()) | ||
| Expect(client.SMembers(ctx, "keyspace_misses").Err()).NotTo(HaveOccurred()) | ||
|
|
||
| newInfo := client.Info(ctx, "stats") | ||
| Expect(newInfo.Err()).NotTo(HaveOccurred()) | ||
| Expect(newInfo.Val()).NotTo(Equal("")) | ||
| Expect(newInfo.Val()).To(ContainSubstring("keyspace_hits")) | ||
| Expect(newInfo.Val()).To(ContainSubstring("keyspace_misses")) | ||
| newInfoKeyspaceHitsStr := extractKeyspaceHits(newInfo.Val(), "keyspace_hits") | ||
| newInfoKeyspaceHits, err := strconv.ParseInt(newInfoKeyspaceHitsStr, 10, 64) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| newInfoKeyspaceMissesStr := extractKeyspaceHits(newInfo.Val(), "keyspace_misses") | ||
| newInfoKeyspaceMisses, err := strconv.ParseInt(newInfoKeyspaceMissesStr, 10, 64) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(newInfoKeyspaceHits - oldInfoKeyspaceHits).To(Equal(int64(1))) | ||
| Expect(newInfoKeyspaceMisses - oldInfoKeyspaceMisses).To(Equal(int64(1))) | ||
|
|
||
| Expect(client.SRem(ctx, "keyspace_hits", "one").Err()).NotTo(HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and error handling
The test case covers the basic functionality of keyspace hits and misses, but we can improve it:
Consider the following enhancements:
- Add error checking for all Redis operations.
- Use
Expect().To(Succeed())for operations where you don't need to check the return value. - Add a check for the initial keyspace hits and misses to ensure they start at expected values.
- Consider testing with multiple set operations to ensure the counts increment correctly for multiple operations.
Here's a partial example of how you might implement these suggestions:
It("should Info keyspace hits", func() {
- sRem := client.SRem(ctx, "keyspace_hits", "one")
- Expect(sRem.Err()).NotTo(HaveOccurred())
- sAdd := client.SAdd(ctx, "keyspace_hits", "one")
- Expect(sAdd.Err()).NotTo(HaveOccurred())
+ Expect(client.SRem(ctx, "keyspace_hits", "one").Err()).To(Succeed())
+ Expect(client.SAdd(ctx, "keyspace_hits", "one").Err()).To(Succeed())
info := client.Info(ctx, "stats")
Expect(info.Err()).NotTo(HaveOccurred())
Expect(info.Val()).NotTo(Equal(""))
Expect(info.Val()).To(ContainSubstring("keyspace_hits"))
Expect(info.Val()).To(ContainSubstring("keyspace_misses"))
oldInfoKeyspaceHitsStr := extractKeyspaceHits(info.Val(), "keyspace_hits")
oldInfoKeyspaceHits, err := strconv.ParseInt(oldInfoKeyspaceHitsStr, 10, 64)
Expect(err).NotTo(HaveOccurred())
oldInfoKeyspaceMissesStr := extractKeyspaceHits(info.Val(), "keyspace_misses")
oldInfoKeyspaceMisses, err := strconv.ParseInt(oldInfoKeyspaceMissesStr, 10, 64)
Expect(err).NotTo(HaveOccurred())
- Expect(client.SMembers(ctx, "keyspace_hits").Err()).NotTo(HaveOccurred())
- Expect(client.SMembers(ctx, "keyspace_misses").Err()).NotTo(HaveOccurred())
+ Expect(client.SMembers(ctx, "keyspace_hits").Err()).To(Succeed())
+ Expect(client.SMembers(ctx, "keyspace_misses").Err()).To(Succeed())
+
+ // Add more set operations to test multiple increments
+ Expect(client.SAdd(ctx, "keyspace_hits", "two").Err()).To(Succeed())
+ Expect(client.SRem(ctx, "keyspace_hits", "nonexistent").Err()).To(Succeed())
newInfo := client.Info(ctx, "stats")
Expect(newInfo.Err()).NotTo(HaveOccurred())
Expect(newInfo.Val()).NotTo(Equal(""))
Expect(newInfo.Val()).To(ContainSubstring("keyspace_hits"))
Expect(newInfo.Val()).To(ContainSubstring("keyspace_misses"))
newInfoKeyspaceHitsStr := extractKeyspaceHits(newInfo.Val(), "keyspace_hits")
newInfoKeyspaceHits, err := strconv.ParseInt(newInfoKeyspaceHitsStr, 10, 64)
Expect(err).NotTo(HaveOccurred())
newInfoKeyspaceMissesStr := extractKeyspaceHits(newInfo.Val(), "keyspace_misses")
newInfoKeyspaceMisses, err := strconv.ParseInt(newInfoKeyspaceMissesStr, 10, 64)
Expect(err).NotTo(HaveOccurred())
- Expect(newInfoKeyspaceHits - oldInfoKeyspaceHits).To(Equal(int64(1)))
- Expect(newInfoKeyspaceMisses - oldInfoKeyspaceMisses).To(Equal(int64(1)))
+ Expect(newInfoKeyspaceHits - oldInfoKeyspaceHits).To(Equal(int64(2))) // 2 successful operations
+ Expect(newInfoKeyspaceMisses - oldInfoKeyspaceMisses).To(Equal(int64(1))) // 1 miss for nonexistent key
- Expect(client.SRem(ctx, "keyspace_hits", "one").Err()).NotTo(HaveOccurred())
+ Expect(client.SRem(ctx, "keyspace_hits", "one", "two").Err()).To(Succeed())
})These changes will make the test more robust and comprehensive.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/integration/server_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
tests/integration/server_test.go (1)
Line range hint
1-595: Overall assessment of changesThe additions to this file are valuable, introducing tests for keyspace hits and misses functionality. However, there are opportunities for improvement:
- The
extractKeyspaceHitsfunction could be optimized for performance and error handling.- The "should Info keyspace hits" test case could be restructured for better readability and robustness.
These changes will enhance the maintainability and reliability of the test suite. Consider implementing the suggested improvements to further strengthen the codebase.
| func extractKeyspaceHits(infoVal string, kWords string) string { | ||
| lines := strings.Split(infoVal, "\n") | ||
| for _, line := range lines { | ||
| if strings.Contains(line, kWords+":") { | ||
| parts := strings.Split(line, ":") | ||
| if len(parts) == 2 { | ||
| return strings.TrimSpace(parts[1]) | ||
| } | ||
| } | ||
| } | ||
| return "0" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the extractKeyspaceHits function
The function logic is correct, but there are some potential improvements:
- Use
strings.HasPrefixinstead ofstrings.Containsfor more precise matching. - Use
strings.SplitNinstead ofstrings.Splitto avoid unnecessary splitting. - Consider returning an error when the key is not found, instead of always returning "0".
Here's a suggested optimization:
func extractKeyspaceHits(infoVal string, kWords string) (string, error) {
lines := strings.Split(infoVal, "\n")
for _, line := range lines {
if strings.HasPrefix(line, kWords+":") {
parts := strings.SplitN(line, ":", 2)
if len(parts) == 2 {
return strings.TrimSpace(parts[1]), nil
}
}
}
return "0", fmt.Errorf("key %s not found", kWords)
}This change would improve performance and allow for better error handling in the calling code.
| It("should Info keyspace hits", func() { | ||
| sRem := client.SRem(ctx, "keyspace_hits", "one") | ||
| Expect(sRem.Err()).NotTo(HaveOccurred()) | ||
| sAdd := client.SAdd(ctx, "keyspace_hits", "one") | ||
| Expect(sAdd.Err()).NotTo(HaveOccurred()) | ||
|
|
||
| info := client.Info(ctx, "stats") | ||
| Expect(info.Err()).NotTo(HaveOccurred()) | ||
| Expect(info.Val()).NotTo(Equal("")) | ||
| Expect(info.Val()).To(ContainSubstring("keyspace_hits")) | ||
| Expect(info.Val()).To(ContainSubstring("keyspace_misses")) | ||
| oldInfoKeyspaceHitsStr := extractKeyspaceHits(info.Val(), "keyspace_hits") | ||
| oldInfoKeyspaceHits, err := strconv.ParseInt(oldInfoKeyspaceHitsStr, 10, 64) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| oldInfoKeyspaceMissesStr := extractKeyspaceHits(info.Val(), "keyspace_misses") | ||
| oldInfoKeyspaceMisses, err := strconv.ParseInt(oldInfoKeyspaceMissesStr, 10, 64) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(client.SMembers(ctx, "keyspace_hits").Err()).NotTo(HaveOccurred()) | ||
| Expect(client.SMembers(ctx, "keyspace_misses").Err()).NotTo(HaveOccurred()) | ||
|
|
||
| newInfo := client.Info(ctx, "stats") | ||
| Expect(newInfo.Err()).NotTo(HaveOccurred()) | ||
| Expect(newInfo.Val()).NotTo(Equal("")) | ||
| Expect(newInfo.Val()).To(ContainSubstring("keyspace_hits")) | ||
| Expect(newInfo.Val()).To(ContainSubstring("keyspace_misses")) | ||
| newInfoKeyspaceHitsStr := extractKeyspaceHits(newInfo.Val(), "keyspace_hits") | ||
| newInfoKeyspaceHits, err := strconv.ParseInt(newInfoKeyspaceHitsStr, 10, 64) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| newInfoKeyspaceMissesStr := extractKeyspaceHits(newInfo.Val(), "keyspace_misses") | ||
| newInfoKeyspaceMisses, err := strconv.ParseInt(newInfoKeyspaceMissesStr, 10, 64) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(newInfoKeyspaceHits - oldInfoKeyspaceHits).To(Equal(int64(1))) | ||
| Expect(newInfoKeyspaceMisses - oldInfoKeyspaceMisses).To(Equal(int64(1))) | ||
|
|
||
| Expect(client.SRem(ctx, "keyspace_hits", "one").Err()).NotTo(HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the "should Info keyspace hits" test case
The test case is comprehensive, but consider the following improvements:
- Use
Expect(...).To(Succeed())for operations where you don't need to check the return value. - Add a check for the initial keyspace hits and misses to ensure they start at expected values.
- Consider testing with multiple set operations to ensure the counts increment correctly for multiple operations.
- Improve cleanup at the end of the test.
Here's a partial example of how you might implement these suggestions:
It("should Info keyspace hits", func() {
// Check initial values
initialInfo := client.Info(ctx, "stats")
Expect(initialInfo.Err()).NotTo(HaveOccurred())
initialHits := extractKeyspaceHits(initialInfo.Val(), "keyspace_hits")
initialMisses := extractKeyspaceHits(initialInfo.Val(), "keyspace_misses")
// Perform operations
Expect(client.SRem(ctx, "keyspace_hits", "one")).To(Succeed())
Expect(client.SAdd(ctx, "keyspace_hits", "one")).To(Succeed())
Expect(client.SAdd(ctx, "keyspace_hits", "two")).To(Succeed())
// Check updated values
newInfo := client.Info(ctx, "stats")
Expect(newInfo.Err()).NotTo(HaveOccurred())
newHits := extractKeyspaceHits(newInfo.Val(), "keyspace_hits")
newMisses := extractKeyspaceHits(newInfo.Val(), "keyspace_misses")
// Assert the changes
Expect(newHits).To(Equal(strconv.FormatInt(mustParseInt(initialHits)+2, 10)))
Expect(newMisses).To(Equal(strconv.FormatInt(mustParseInt(initialMisses)+1, 10)))
// Cleanup
Expect(client.Del(ctx, "keyspace_hits")).To(Succeed())
})
func mustParseInt(s string) int64 {
i, err := strconv.ParseInt(s, 10, 64)
if err != nil {
panic(err)
}
return i
}This structure makes the test more robust and easier to maintain.
修改内容
1.新增keyspace_hits、keyspace_misses指标
2.修改了set相关命令的返回结果，使得keyspace_hits、keyspace_misses可以正确统计
3.新增了go测试
需要审查的点
说明
这次改动主要为了在pika支持keyspace hit相关参数。 关联的issue： #2423
参考redis的实现: redis在执行操作前会统一在内存查找key是否存在并统计
但在pika这一层，需要在各个命令执行阶段实现统计。
即，当key不存在时s_.IsNotFound()==true。
当前只修改set，后续还需要将其他的数据类型命令全部修改。
异常返回统计表
当前版本由于storage层部分s_.IsNotFound()接口返回结果不适配，导致部分命令的返回结果会超出预期，这里是相关列表。
Summary by CodeRabbit
New Features
Bug Fixes
Tests