feat: support triple grpc heart beat #1432
Conversation
WalkthroughThe recent changes primarily enhance gRPC client configuration in the RPC framework. A new constant for setting the keep-alive interval has been added, along with corresponding configuration keys and default values. Additionally, a static variable has been introduced in the transport layer to utilize this interval during channel initialization. Unit tests have also been created to validate these changes, ensuring correct default value assignments and overall reliability in connection management. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Config as Configurations
participant Transport as TripleClientTransport
App->>Config: Request keep-alive interval
Config-->>App: Return keep-alive interval (default 0)
App->>Transport: Initialize transport with keep-alive interval
Transport-->>Transport: Set channel parameters based on interval
Transport-->>App: Transport ready with configured settings
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
Outside diff range, codebase verification and nitpick comments (1)
remoting/remoting-triple/src/test/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransportTest.java (1)
29-34: Test case verifies default value of KEEP_ALIVE_INTERVAL.The test case correctly verifies the default value of the
KEEP_ALIVE_INTERVALconstant. However, consider adding more test cases to cover different scenarios, such as non-default values and edge cases.@Test public void testNonDefaultKeepAliveInterval() { // Assuming there's a way to set a custom value for testing int customInterval = 30; TripleClientTransport.setKeepAliveInterval(customInterval); Assert.assertEquals(TripleClientTransport.KEEP_ALIVE_INTERVAL, customInterval); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- core/api/src/main/java/com/alipay/sofa/rpc/common/RpcOptions.java (2 hunks)
- core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (1 hunks)
- core/common/src/main/resources/com/alipay/sofa/rpc/common/rpc-config-default.json (1 hunks)
- remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransport.java (3 hunks)
- remoting/remoting-triple/src/test/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransportTest.java (1 hunks)
Additional comments not posted (11)
remoting/remoting-triple/src/test/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransportTest.java (5)
1-16: Ensure proper licensing and author information.The license header is correctly included. Ensure that the author information is accurate and up-to-date.
17-17: Package declaration is correct.The package declaration aligns with the directory structure.
19-20: Imports are minimal and relevant.The imports are relevant to the test case.
22-26: Class-level Javadoc is clear and concise.The Javadoc provides a clear description of the class and its purpose.
27-27: Class declaration is appropriate.The class declaration is appropriate for a test class.
core/common/src/main/resources/com/alipay/sofa/rpc/common/rpc-config-default.json (1)
276-277: New configuration option for gRPC client keep-alive interval.The new configuration option for the gRPC client keep-alive interval is correctly added and follows the existing structure and conventions. Ensure that this new option is documented and communicated to users.
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransport.java (2)
83-86: New static variableKEEP_ALIVE_INTERVALinitialization.The new static variable
KEEP_ALIVE_INTERVALis correctly initialized usingSofaConfigs.getOrCustomDefault(). Ensure that the default value is appropriate and that this variable is used consistently throughout the class.
289-292: Conditional keep-alive configuration ininitChannelmethod.The
initChannelmethod correctly configures the keep-alive behavior based on the value ofKEEP_ALIVE_INTERVAL. This change enhances connection management by enabling keep-alive settings when the interval is greater than zero.core/common/src/main/java/com/alipay/sofa/rpc/common/config/RpcConfigKeys.java (1)
143-154: LGTM! Ensure the usage of the new configuration key in the codebase.The new configuration key
TRIPLE_CLIENT_KEEP_ALIVE_INTERVALis correctly defined with a default value of0, indicating that the keep-alive feature is not enabled unless explicitly configured.However, ensure that this new configuration key is properly utilized in the codebase.
Verification successful
The new configuration key
TRIPLE_CLIENT_KEEP_ALIVE_INTERVALis correctly utilized in the codebase.The key is used to set the keep-alive interval for the gRPC client in the
TripleClientTransportclass, ensuring the feature is enabled only when explicitly configured.
TripleClientTransport.java: TheKEEP_ALIVE_INTERVALis conditionally applied to the gRPC client builder.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new configuration key `TRIPLE_CLIENT_KEEP_ALIVE_INTERVAL`. # Test: Search for the configuration key usage. Expect: Proper utilization in the codebase. rg --type java $'TRIPLE_CLIENT_KEEP_ALIVE_INTERVAL'Length of output: 425
Script:
#!/bin/bash # Description: Extract context around the usage of `TRIPLE_CLIENT_KEEP_ALIVE_INTERVAL` in `TripleClientTransport.java`. # Extract 10 lines before and after the match to understand the context. rg --type java -A 10 -B 10 $'TRIPLE_CLIENT_KEEP_ALIVE_INTERVAL' remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransport.javaLength of output: 979
Script:
#!/bin/bash # Description: Search for all occurrences of `KEEP_ALIVE_INTERVAL` within the `TripleClientTransport.java` file. rg --type java $'KEEP_ALIVE_INTERVAL' remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransport.javaLength of output: 563
core/api/src/main/java/com/alipay/sofa/rpc/common/RpcOptions.java (2)
28-421: Approved: Formatting changes enhance readability.The formatting changes normalize the spacing around the equal sign (
=) for existing constant string declarations, enhancing readability and maintainability by ensuring uniformity in style.
423-426: LGTM! Ensure the usage of the new constant string declaration in the codebase.The new constant string declaration
TRANSPORT_GRPC_CLIENT_KEEP_ALIVE_INTERVALis correctly defined and specifies a configuration option related to the gRPC client's keep-alive interval.However, ensure that this new constant string declaration is properly utilized in the codebase.
Verification successful
The new constant string declaration
TRANSPORT_GRPC_CLIENT_KEEP_ALIVE_INTERVALis correctly defined and utilized in the codebase.
- The constant is defined in
core/api/src/main/java/com/alipay/sofa/rpc/common/RpcOptions.java.- It is used in
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransport.java.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new constant string declaration `TRANSPORT_GRPC_CLIENT_KEEP_ALIVE_INTERVAL`. # Test: Search for the constant string declaration usage. Expect: Proper utilization in the codebase. rg --type java $'TRANSPORT_GRPC_CLIENT_KEEP_ALIVE_INTERVAL'Length of output: 477
| @@ -273,6 +273,8 @@ PS:大家也看到了，本JSON文档是支持注释的，而标准JSON是不支 | |||
| "compress.size.baseline": 2048, | |||
| //Whether the Http2 Cleartext protocol client uses Prior Knowledge to start Http2 | |||
| "transport.client.h2c.usePriorKnowledge": true, | |||
| // grpc client keep alive interval, default to 0, no keep alive | |||
| "transport.grpc.client.keepAlive.interval": 0, | |||
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
core/common/src/main/resources/com/alipay/sofa/rpc/common/rpc-config-default.json (1)
276-277: LGTM! Consider adding units to the comment.The new configuration for gRPC client keep-alive interval is well-placed and follows the existing naming conventions. The default value and its meaning are clearly explained in the comment.
Consider adding the time unit (e.g., milliseconds, seconds) to the comment for clarity. For example:
- // grpc client keep alive interval, default to 0, no keep alive + // grpc client keep alive interval in milliseconds, default to 0 (no keep alive) "sofa.rpc.triple.client.keepAlive.interval": 0,remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransport.java (2)
83-85: LGTM: New static variable for keep-alive interval.The
KEEP_ALIVE_INTERVALis well-implemented, using configuration systems for flexibility. Consider adding a comment explaining the purpose and unit of this interval for better clarity.Add a comment above the variable declaration:
/** * The keep-alive interval in seconds for the triple gRPC protocol. * A value of 0 means the keep-alive feature is disabled. */ protected static int KEEP_ALIVE_INTERVAL = ...
287-292: LGTM: Keep-alive configuration added correctly.The implementation of the keep-alive feature aligns well with the PR objectives. It correctly checks the
KEEP_ALIVE_INTERVALand applies the settings when appropriate.Consider adding a maximum limit to the keep-alive interval for added robustness:
if (KEEP_ALIVE_INTERVAL > 0) { builder.keepAliveWithoutCalls(true); - builder.keepAliveTime(KEEP_ALIVE_INTERVAL, TimeUnit.SECONDS); + builder.keepAliveTime(Math.min(KEEP_ALIVE_INTERVAL, 300), TimeUnit.SECONDS); }This ensures that the keep-alive interval doesn't exceed 5 minutes (300 seconds), which is a common maximum value for such settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- core/api/src/main/java/com/alipay/sofa/rpc/common/RpcOptions.java (1 hunks)
- core/common/src/main/resources/com/alipay/sofa/rpc/common/rpc-config-default.json (1 hunks)
- remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransport.java (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/api/src/main/java/com/alipay/sofa/rpc/common/RpcOptions.java
🔇 Additional comments (2)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/transport/triple/TripleClientTransport.java (2)
19-19: LGTM: Import statement added correctly.The new import for
com.alipay.sofa.common.config.SofaConfigsis appropriately placed and necessary for the new functionality.
Line range hint
1-353: Overall: Well-implemented feature for triple gRPC heartbeat support.The changes effectively implement the triple gRPC heartbeat feature as described in the PR objectives. The code is well-structured, uses appropriate configuration systems, and follows good practices. The minor suggestions provided will further enhance the clarity and robustness of the implementation.
Motivation:
Enable heart beat for triple so as to archive tcp keep alive.
Notice that when we apply MOSN, there is the mechanism that close inactive connection.
Modification:
Add config that set the keep alive interval.
Result:
Allow config for keep alive interval for triple. Default at 0. not enabled.
Summary by CodeRabbit
New Features
Bug Fixes
Tests