fix: set default 0 when get method timeout config #1422
Conversation
sofastack-cla
bot
added
bug
WalkthroughThe changes improve timeout management in the codebase. They modify the Changes
Sequence Diagram(s)Diagrams are not necessary as the changes are primarily related to configuration and testing enhancements. Poem
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- core/api/src/main/java/com/alipay/sofa/rpc/config/MethodConfig.java (1 hunks)
- core/api/src/test/java/com/alipay/sofa/rpc/std/config/AbstractInterfaceConfigTest.java (2 hunks)
- registry/registry-sofa/src/test/java/com/alipay/sofa/rpc/registry/sofa/SofaRegistryHelperTest.java (1 hunks)
Files skipped from review due to trivial changes (1)
- core/api/src/main/java/com/alipay/sofa/rpc/config/MethodConfig.java
Additional comments not posted (1)
core/api/src/test/java/com/alipay/sofa/rpc/std/config/AbstractInterfaceConfigTest.java (1)
26-26: The import ofProviderConfigis appropriate as it supports the new test method added.
|
|
||
| ProviderConfig<?> providerConfig2 = new ProviderConfig<>(); | ||
| providerConfig2.setMethods(new HashMap<>()); | ||
| providerConfig2.getMethods().put("test", new MethodConfig().setTimeout(null)); | ||
| Assert.assertNotNull(providerConfig2.getTimeout()); | ||
| String s4 = SofaRegistryHelper.convertProviderToUrls(providerConfig2, serverConfig); | ||
| Assert.assertNotNull(s3); | ||
| ProviderInfo providerInfo4 = SofaRegistryHelper.parseProviderInfo(s4); | ||
| Assert.assertEquals(0, providerInfo4.getDynamicAttr(".test.timeout")); | ||
| Assert.assertFalse(providerConfig2.hasTimeout()); | ||
|
|
There was a problem hiding this comment.
The modifications in the convertProviderToUrls method correctly handle the scenario where a method's timeout is null, ensuring a default value of 0 is used. This aligns with the PR's objectives to prevent null timeout values from being published. Consider verifying this behavior in integration tests to ensure it behaves as expected across different configurations.
Would you like assistance in setting up integration tests for this scenario?
| @Test | ||
| public void testMethodTimeout() { | ||
| MethodConfig config = new MethodConfig(); | ||
| config.setTimeout(null); | ||
|
|
||
| ProviderConfig p = new ProviderConfig(); | ||
| p.setMethods(new HashMap<>()); | ||
| p.getMethods().put("test", config); | ||
|
|
||
| try { | ||
| Assert.assertFalse(p.hasTimeout()); | ||
| } catch (Exception e) { | ||
| Assert.fail("exception should not appears: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding more test scenarios for robust testing.
The test method testMethodTimeout effectively checks the behavior when timeout is null. Consider adding more scenarios to ensure robust testing, such as verifying the actual timeout value used by the system when null is set.
Would you like me to help create additional test cases for this scenario?
|
I think we still need to discuss the following points to reach a consensus:
|
@OrezzerO And, for each of the above step, only when a positive value appears will it stop and active,
Secondly, I dont think "no timeout" which lead to endless blocking is appropriate within this scope. As we dont want to witness thread leakage due to blocking and waiting for response that might not come due to some error. So, in my opinion, value no greater than 0 is, actually, meaningless and should be replaced with default value. |
|
First, I contend that this PR is "acceptable" because it doesn't alter the existing logic. Regarding the first point I mentioned earlier:As you stated:
Regarding the second point I mentioned earlier:When users set 0 for the timeout, there could be three scenarios:
To draw a conclusion, I have the following suggestions for this PR: BTW , consider what will happen when default timeout is 0 ? The client won't wait. The default timeout and user defined timeout acts differently. |
@OrezzerO Welp, actually the whole project already made its choice. As for default or 0 valueAbsolutely, so it will not currently be exposed to the public, there is the logic to ensure that: if (providerConfig.getTimeout() > 0) {
sb.append(getKeyPairs(ATTR_TIMEOUT, providerConfig.getTimeout()));
}
I guess this is designed for the However, the client side will always set a 0 value if there is no meaningful value (cuz it is a This mechanism achieves that For the second concernSo now we got the conclusion, |
Motivation:
The method-level
timeoutconfig isIntegertype which acceptsnullvalue.So if we add method config and set the method-level
timeoutto null, atimeout=nullmight be pub onto registry.Say,
1.1.1.1:12200?rpcVer=12300&serialization=hessian&app_name=demo&[echo]=[clientTimeout#null].When dealing with
nullvalue, some of the implementations may omit catching exception when doingInteger.parseInt(null), leading to unexpected startup error.Modification:
As both
nulland0are meaningless in SOFARPC (might only indicate 'not set', but we do not care), I setmethod.getTimeouta default 0 return value ifnull.Result:
Focus on
com.alipay.sofa.rpc.registry.sofa.SofaRegistryHelper#convertProviderToUrlscom.alipay.sofa.rpc.registry.mesh.SofaRegistryHelper#convertProviderToUrlswill not get and pub a
nulltimeout value any more.Summary by CodeRabbit
Bug Fixes
0when not set.Tests