hzhaop commented on code in PR #777:
URL: https://github.com/apache/skywalking-java/pull/777#discussion_r2509288503
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/Config.java:
##########
@@ -210,6 +210,20 @@ public static class Collector {
* How long grpc client will timeout in sending data to upstream.
*/
public static int GRPC_UPSTREAM_TIMEOUT = 30;
+ /**
+ * The interval in seconds to send a keepalive ping to the backend.
+ * If this is less than or equal to 0, the keepalive is disabled.
+ *
Review Comment:
> [nitpick] The `GRPC_KEEPALIVE_TIME` field is declared as `long` but the
configuration comment in agent.config (line 105) uses it with TimeUnit.SECONDS
in GRPCChannel.java (line 45). The configuration validation at line 44 in
GRPCChannel checks if `> 0`, but according to gRPC documentation, keepalive
time values below a certain threshold (typically 10 seconds) may be rejected by
the server. Consider adding a comment documenting the minimum safe value or
adding validation.
Thanks for the suggestion! I've added the documentation for the minimum safe
keepalive time value in the following locations:
1. **Config.java** (line 218): Added a note in the Javadoc for
`GRPC_KEEPALIVE_TIME`:
```java
* <p>
* <b>Note:</b> The minimum safe value is 10 seconds. Values below this
may be rejected by the gRPC server.
```
2. **agent.config** (line 105): Added a comment in the configuration file:
```properties
# Note: The minimum safe value is 10 seconds. Values below this may be
rejected by the gRPC server.
```
I opted for documentation rather than validation because:
- The default value (120 seconds) is already well above the minimum threshold
- Users who explicitly configure a lower value may have specific use cases
or testing scenarios
Let me know if you think validation should still be added!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
