feat:bigkey fuse degration based on codis-proxy #2783
Conversation
WalkthroughThe overall changes introduce a robust system to manage and monitor Redis components. This includes functionality for checking request/response sizes and contents, an advanced circuit breaker for handling service degradation, and comprehensive management of command and key blacklists. Additionally, configuration settings for these features are added and validated, with corresponding proxy and session handling adjustments made. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant Redis
Client->>Proxy: Send Request
Proxy->>Checker: Check Request (size, content)
alt Check Failed
Checker-->>Proxy: Request Invalid
Proxy-->>Client: Return Error
else Check Passed
Checker-->>Proxy: Request Valid
Proxy->>Breaker: Check Service Degradation
alt Degradation Enabled
Breaker-->>Proxy: Service Rejected
Proxy-->>Client: Return Degradation Response
else No Degradation
Breaker-->>Proxy: Service Allowed
Proxy->>Redis: Forward Valid Request
Redis-->>Proxy: Return Response
Proxy->>Checker: Check Response (content, size)
alt Response Check Failed
Checker-->>Proxy: Response Invalid
Proxy-->>Client: Return Error
else Response Check Passed
Checker-->>Proxy: Response Valid
Proxy-->>Client: Return Response
end
end
end
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 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: 2
Outside diff range and nitpick comments (7)
codis/pkg/proxy/CheckCustumFunc.go (7)
23-25: Clarify the purpose of CheckResponse method.The
CheckResponsemethod always returns false. Clarify if this is the intended behavior or if additional logic is needed.
29-31: Clarify the purpose of CheckRequest method.The
CheckRequestmethod always returns false. Clarify if this is the intended behavior or if additional logic is needed.
61-63: Clarify the purpose of CheckRequest method.The
CheckRequestmethod always returns false. Clarify if this is the intended behavior or if additional logic is needed.
82-84: Clarify the purpose of CheckRequest method.The
CheckRequestmethod always returns false. Clarify if this is the intended behavior or if additional logic is needed.
297-299: Clarify the purpose of CheckResponse method.The
CheckResponsemethod always returns false. Clarify if this is the intended behavior or if additional logic is needed.
315-317: Clarify the purpose of CheckResponse method.The
CheckResponsemethod always returns false. Clarify if this is the intended behavior or if additional logic is needed.
336-338: Clarify the purpose of CheckResponse method.The
CheckResponsemethod always returns false. Clarify if this is the intended behavior or if additional logic is needed.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- codis/pkg/proxy/CheckBkv.go (1 hunks)
- codis/pkg/proxy/CheckCustumFunc.go (1 hunks)
- codis/pkg/proxy/breaker.go (1 hunks)
- codis/pkg/proxy/breaker_black_list.go (1 hunks)
- codis/pkg/proxy/config.go (3 hunks)
- codis/pkg/proxy/mapper.go (5 hunks)
- codis/pkg/proxy/mapper_test.go (2 hunks)
- codis/pkg/proxy/proxy.go (2 hunks)
- codis/pkg/proxy/request.go (1 hunks)
- codis/pkg/proxy/session.go (5 hunks)
Additional comments not posted (55)
codis/pkg/proxy/request.go (1)
23-24: Ensure proper initialization of new fields.The new fields
OpFlagCheckerandCustomCheckFuncshould be properly initialized to avoid potential nil pointer dereferences.codis/pkg/proxy/breaker.go (4)
9-13: LGTM! Ensure proper usage of new variables.The new variables
ErrMsgServiceRejected,breakerSwitch, andprobabilityOfServiceDegradationare correctly declared.
21-26: LGTM! Ensure the state is set correctly.The
BreakerSetStatefunction correctly sets the breaker switch state and handles invalid input.
29-37: LGTM! Ensure the probability is set correctly.The
BreakerSetProbabilityfunction correctly sets the degradation probability and handles invalid input.
53-79: Ensure comprehensive testing of service degradation logic.The
IfDegradateServicefunction introduces complex logic for service degradation based on various conditions. Ensure comprehensive testing to cover all possible scenarios.codis/pkg/proxy/breaker_black_list.go (3)
14-19: LGTM! Ensure proper usage of new variables.The new variables
cmdBlackListSwitch,keyBlackListSwitch,cmdBlackList, andkeyBlackListare correctly declared.
104-121: Ensure proper handling of key blacklist checks.The
CheckKeyBlackListfunction correctly checks the key against the blacklist, including partial matches. Ensure that the function handles edge cases, such as empty or invalid input.
81-88: Ensure proper handling of key blacklist batch processing.The
StoreKeyBlackListByBatchfunction correctly handles the batch processing and updates the switch state. Ensure that the function handles edge cases, such as empty or invalid input.codis/pkg/proxy/mapper_test.go (1)
23-25: Ensure test cases handle new return values correctly.The
TestGetOpStrfunction has been modified to return four strings instead of two. Ensure that the test cases correctly handle the new return values.codis/pkg/proxy/CheckCustumFunc.go (16)
64-77: Ensure value size check logic is correct.The
CheckResponsemethod checks if the value size is greater than or equal toGetMaxValueLength(). Ensure this logic aligns with the intended behavior.
110-122: Ensure count value check logic is correct.The function
checkScanRequestchecks if the count value is greater than or equal toGetMaxBatchSize(). Ensure this logic aligns with the intended behavior.
32-56: Ensure response length and value size check logic is correct.The
CheckResponsemethod checks if the response length and value sizes are within limits. Ensure this logic aligns with the intended behavior.
10-22: Ensure value length check logic is correct.The
CheckRequestmethod checks if the value length is greater than or equal toGetMaxValueLength(). Ensure this logic aligns with the intended behavior.
126-128: EnsurecheckScanRequestlogic is correct.The
CheckRequestmethod usescheckScanRequest. EnsurecheckScanRequestlogic aligns with the intended behavior.
294-296: EnsurecheckRangeRequestlogic is correct.The
CheckRequestmethod usescheckRangeRequest. EnsurecheckRangeRequestlogic aligns with the intended behavior.
321-335: Ensure count value check logic is correct.The
CheckRequestmethod checks if the count value is greater than or equal toGetMaxBatchSize(). Ensure this logic aligns with the intended behavior.
302-314: Ensure count value check logic is correct.The
CheckRequestmethod checks if the count value is greater than or equal toGetMaxBatchSize(). Ensure this logic aligns with the intended behavior.
180-200: Ensure batch size check logic is correct.The
CheckResponsemethod checks if the batch size is greater than or equal toGetMaxBatchSize(). Ensure this logic aligns with the intended behavior.
85-104: Ensure batch size check logic is correct.The
CheckResponsemethod checks if the batch size is greater than or equal toGetMaxBatchSize(). Ensure this logic aligns with the intended behavior.
204-206: EnsurecheckScanRequestlogic is correct.The
CheckRequestmethod usescheckScanRequest. EnsurecheckScanRequestlogic aligns with the intended behavior.
207-225: Ensure batch size check logic is correct.The
CheckResponsemethod checks if the batch size is greater than or equal toGetMaxBatchSize(). Ensure this logic aligns with the intended behavior.
261-263: EnsurecheckRangeRequestlogic is correct.The
CheckRequestmethod usescheckRangeRequest. EnsurecheckRangeRequestlogic aligns with the intended behavior.
264-289: Ensure batch size check logic is correct.The
CheckResponsemethod checks if the batch size is greater than or equal toGetMaxBatchSize(). Ensure this logic aligns with the intended behavior.
129-173: Ensure response size check logic is correct.The
CheckResponsemethod checks if the response size is greater than or equal toGetMaxBatchSize()orGetMaxValueLength(). Ensure this logic aligns with the intended behavior.
177-179: EnsurecheckScanRequestlogic is correct.The
CheckRequestmethod usescheckScanRequest. EnsurecheckScanRequestlogic aligns with the intended behavior.codis/pkg/proxy/session.go (7)
347-348: LGTM! Ensure proper integration of "XCONFIG" command handling.The added logic for handling the "XCONFIG" command looks good. Ensure that all necessary integrations and dependencies are properly handled.
332-342: LGTM! Ensure proper integration of fuse degradation logic.The added logic for fuse degradation based on
IfDegradateServicelooks good. Ensure that all necessary integrations and dependencies are properly handled.
379-381: LGTM! Ensure proper integration of fuse degradation in the default case.The added logic for fuse degradation in the default case looks good. Ensure that all necessary integrations and dependencies are properly handled.
Line range hint
354-381: LGTM! Ensure proper integration of fuse degradation in command cases.The added logic for fuse degradation in multiple command cases looks good. Ensure that all necessary integrations and dependencies are properly handled.
443-481: LGTM! Ensure proper integration of "XCONFIG" subcommands handling.The added logic for handling "XCONFIG" subcommands looks good. Ensure that all necessary integrations and dependencies are properly handled.
301-309: LGTM! Ensure proper integration of request checks.The added logic for request checking and custom check functions looks good. Ensure that all necessary integrations and dependencies are properly handled.
244-250: LGTM! Ensure proper integration of response checks.The added logic for response checking and custom check functions looks good. Ensure that all necessary integrations and dependencies are properly handled.
codis/pkg/proxy/proxy.go (1)
436-512: LGTM! Ensure proper integration of new configuration parameters inConfigSet.The added cases for new configuration parameters related to checker and breaker systems look good. Ensure that all necessary integrations and dependencies are properly handled.
codis/pkg/proxy/CheckBkv.go (15)
9-18: LGTM!The constants are well-defined and appropriate for BigKey detection.
20-25: LGTM!The variables are well-defined and appropriate for managing BigKey detection state.
27-29: LGTM!The
Checkertype is well-defined and appropriate for managing the enabled state.
31-33: LGTM!The function correctly checks if the checker is enabled.
34-39: LGTM!The function correctly sets the state of the checker, ensuring it is either 0 or 1.
41-46: LGTM!The function correctly sets the maximum length of a value, ensuring it is non-negative.
48-53: LGTM!The function correctly sets the maximum batch size, ensuring it is non-negative.
56-63: LGTM!The function correctly sets the result set size, ensuring it is within bounds.
66-68: LGTM!The function correctly sets the maximum value length.
69-71: LGTM!The function correctly retrieves the maximum value length.
72-74: LGTM!The function correctly sets the maximum batch size.
75-77: LGTM!The function correctly retrieves the maximum batch size.
78-80: LGTM!The function correctly sets the maximum result set size.
81-83: LGTM!The function correctly retrieves the maximum result set size.
85-93: LGTM!The function correctly checks if a request is a BigRequest based on batch size or content.
However, ensure that the logic for checking batch size and content is correct and covers all edge cases.
codis/pkg/proxy/config.go (3)
147-165: LGTM!The new configuration parameters for the checker and breaker systems are well-defined and appropriate.
233-241: LGTM!The new fields in the
Configstruct are well-defined and appropriate.
383-397: LGTM!The validation logic for the new checker and breaker settings is well-defined and appropriate.
codis/pkg/proxy/mapper.go (4)
69-94: LGTM!The new constants are well-defined and appropriate for request and response checks.
35-35: LGTM!The new type
OpFlagCheckerand its methods are well-defined and appropriate for checking various request and response flags.Also applies to: 96-120
122-125: LGTM!The new interface
CustomCheckFuncis well-defined and appropriate for custom request and response checks.
Line range hint
348-374: LGTM!The function correctly includes logic for returning
OpFlagCheckerandCustomCheckFunc.
| case "checker_enabled": | ||
| return redis.NewBulkBytes([]byte(strconv.FormatInt(p.config.CheckerEnabled, 10))) | ||
| case "checker_max_value_len": | ||
| return redis.NewBulkBytes([]byte(strconv.FormatInt(p.config.CheckerMaxValueLen, 10))) | ||
| case "checker_max_batchsize": | ||
| return redis.NewBulkBytes([]byte(strconv.FormatInt(p.config.CheckerMaxBatchsize, 10))) | ||
| case "checker_result_set_size": | ||
| return redis.NewBulkBytes([]byte(strconv.FormatInt(p.config.CheckerResultSetSize, 10))) | ||
| case "breaker_enabled": | ||
| return redis.NewBulkBytes([]byte(strconv.FormatInt(p.config.BreakerEnabled, 10))) | ||
| case "breaker_degradation_probability": | ||
| return redis.NewBulkBytes([]byte(strconv.FormatInt(p.config.BreakerDegradationProbability, 10))) | ||
| case "breaker_cmd_black_list": | ||
| return redis.NewBulkBytes([]byte(p.config.BreakerCmdBlackList)) | ||
| case "breaker_key_black_list": | ||
| UpdateKeyBlackList(p) |
There was a problem hiding this comment.
Tip
Codebase Verification
Tests Needed for New Configuration Parameters
The new configuration parameters related to checker and breaker systems do not appear to be covered by existing tests. Please add tests to ensure these parameters are properly integrated and functioning as expected.
checker_enabledchecker_max_value_lenchecker_max_batchsizechecker_result_set_sizebreaker_enabledbreaker_degradation_probabilitybreaker_cmd_black_listbreaker_key_black_list
Analysis chain
LGTM! Ensure proper integration of new configuration parameters in ConfigGet.
The added cases for new configuration parameters related to checker and breaker systems look good. Ensure that all necessary integrations and dependencies are properly handled.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of new configuration parameters in `ConfigGet`.
# Test: Search for the usage of `ConfigGet` to ensure it is properly integrated.
rg --type go 'ConfigGet\('
Length of output: 1167
Script:
#!/bin/bash
# Description: Verify the testing of new configuration parameters in `ConfigGet`.
# Test: Search for the new configuration parameters in test files.
rg --type go 'checker_enabled|checker_max_value_len|checker_max_batchsize|checker_result_set_size|breaker_enabled|breaker_degradation_probability|breaker_cmd_black_list|breaker_key_black_list' tests/
Length of output: 201
Script:
#!/bin/bash
# Description: Verify the testing of new configuration parameters in `ConfigGet`.
# Test: Search for test functions that might indirectly test the new configuration parameters.
rg --type go -A 5 'func Test' tests/
Length of output: 988
| func StoreCmdBlackListByBatch(cmdList string) { | ||
| if len(cmdList) <= 0 { //If the list is empty, set the switch to off | ||
| cmdBlackListSwitch.Set(SWITCH_CLOSED) | ||
| } else { //If the list is not empty, set the switch to On | ||
| cmdBlackListSwitch.Set(SWITCH_OPEN) | ||
| } | ||
| for _, cmdName := range strings.Split(cmdList, ",") { | ||
| cmdName = strings.TrimSpace(cmdName) | ||
| StoreCmdBlackList(cmdName) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Add tests for StoreCmdBlackListByBatch function
The StoreCmdBlackListByBatch function currently lacks test coverage, which means its behavior, especially in edge cases, is not verified. Please add unit tests to ensure that the function handles various scenarios correctly, including empty and invalid inputs.
- File:
codis/pkg/proxy/breaker_black_list.go - Function:
StoreCmdBlackListByBatch
Analysis chain
Ensure proper handling of command blacklist batch processing.
The StoreCmdBlackListByBatch function correctly handles the batch processing and updates the switch state. Ensure that the function handles edge cases, such as empty or invalid input.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper handling of command blacklist batch processing.
# Test: Search for test cases covering the StoreCmdBlackListByBatch function. Expect: Proper handling of edge cases.
rg --type go $'StoreCmdBlackListByBatch'
Length of output: 39
Script:
#!/bin/bash
# Description: Verify proper handling of command blacklist batch processing.
# Search for any test cases related to StoreCmdBlackListByBatch function.
rg 'StoreCmdBlackListByBatch' --type go
# Search for test functions that might indirectly test StoreCmdBlackListByBatch.
rg 'func Test' --type go
Length of output: 1331
Script:
#!/bin/bash
# Description: Inspect the contents of test functions to check if they indirectly test StoreCmdBlackListByBatch.
# List the contents of the test functions found.
rg 'func TestPikaWithCache' --type go --context 10
rg 'func TestPikaWithoutCache' --type go --context 10
rg 'func TestExporter_Describe' --type go --context 10
rg 'func Test_Parse_Info' --type go --context 10
rg 'func Test_Parse_Version_Error' --type go --context 10
rg 'func Test_Parse_Proxy_Stats' --type go --context 10
rg 'func TestBefore' --type go --context 10
rg 'func TestPutKeyToPika' --type go --context 10
rg 'func TestListBigKey' --type go --context 10
rg 'func TestCompressKeyAndDeCompress' --type go --context 10
rg 'func TestAfter' --type go --context 10
rg 'func TestHeap' --type go --context 10
rg 'func TestNewCodisDiscovery' --type go --context 10
rg 'func TestCrc16' --type go --context 10
rg 'func TestCalcKeys' --type go --context 10
rg 'func TestKeyHash' --type go --context 10
Length of output: 22120
关联issue： #2228 #1691
BigKey检测逻辑
BigKey检测分两个阶段，
第一阶段检测Request，若Request被检测为大请求则直接被熔断掉
第二阶段检测Response，主要是记录一些bigkey
CheckerBkv中的
CheckerRequest首先会通过OpInfo的flag位调用CheckerRequest
若OpInfo中有特殊化检测（实现了CustomCheckFunc）则还会额外进行一次检测
CheckerCustumFunc
如果需要检测特定命令,则只需要在OpInfo中塞入对应结构体,并为其实现
CustomCheckFunc接口即可熔断
首先需要开启
Checker,通过命令xconfig set checker_enabled 1此时每个命令对应的
session将会开启检测,当检测到bigkey,将会存入黑名单keyblacklist随后通过命令
xconfig set breaker_enabled 1开启熔断通过
xconfig set breaker_degradation_probability %d可以设置熔断概率,0-100当命令进入到
handleRequest()时,会首先进行isBigRequest检测,此处不会熔断,只是设置isBigRequest标识除一些特殊命令以外,其他命令会进入到熔断检测
IfDegradateService()ExecuteServiceDegradation(r, rd)负责执行熔断策略并返回熔断信息Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores
getOpInfofunction.