Copilot commented on code in PR #10710:
URL: https://github.com/apache/cloudstack/pull/10710#discussion_r2309193464
##########
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##########
@@ -1470,14 +1420,15 @@ private GetRouterMonitorResultsAnswer
fetchAndUpdateRouterHealthChecks(DomainRou
try {
final Answer answer = _agentMgr.easySend(router.getHostId(),
command);
+ logger.info("Got health check results from router {}: {}",
router.getHostName(), answer != null ? answer.getDetails() : "null answer");
Review Comment:
This info-level logging of health check results on every execution could
generate excessive log volume in production. Consider using debug level or
adding conditional logging based on result status.
```suggestion
logger.debug("Got health check results from router {}: {}",
router.getHostName(), answer != null ? answer.getDetails() : "null answer");
```
##########
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##########
@@ -3259,18 +3182,18 @@ public void doInTransactionWithoutResult(final
TransactionStatus status) {
}
if (stats.getCurrentBytesReceived() >
answerFinal.getBytesReceived()) {
- if (logger.isDebugEnabled()) {
- logger.debug("Received # of bytes
that's less than the last one. " + "Assuming something went wrong and
persisting it. Router: "
- +
answerFinal.getRouterName() + " Reported: " +
toHumanReadableSize(answerFinal.getBytesReceived()) + " Stored: " +
toHumanReadableSize(stats.getCurrentBytesReceived()));
- }
+ logger.debug("Received # of bytes
that's less than the last one. Assuming something went wrong and persisting it.
Router: {} Reported: {} Stored: {}"
+ ,
answerFinal.getRouterName()
+ ,
toHumanReadableSize(answerFinal.getBytesReceived())
+ ,
toHumanReadableSize(stats.getCurrentBytesReceived()));
stats.setNetBytesReceived(stats.getNetBytesReceived() +
stats.getCurrentBytesReceived());
}
stats.setCurrentBytesReceived(answerFinal.getBytesReceived());
if (stats.getCurrentBytesSent() >
answerFinal.getBytesSent()) {
- if (logger.isDebugEnabled()) {
- logger.debug("Received # of bytes
that's less than the last one. " + "Assuming something went wrong and
persisting it. Router: "
- +
answerFinal.getRouterName() + " Reported: " +
toHumanReadableSize(answerFinal.getBytesSent()) + " Stored: " +
toHumanReadableSize(stats.getCurrentBytesSent()));
- }
+ logger.debug("Received # of bytes
that's less than the last one. Assuming something went wrong and persisting it.
Router: {} Reported: {} Stored: {}"
+ , answerFinal.getRouterName()
+ ,
toHumanReadableSize(answerFinal.getBytesReceived())
+ ,
toHumanReadableSize(stats.getCurrentBytesReceived()));
Review Comment:
The log message uses getBytesReceived() twice but should use getBytesSent()
for the second parameter to match the log message about sent bytes.
```suggestion
,
toHumanReadableSize(answerFinal.getBytesSent())
,
toHumanReadableSize(stats.getCurrentBytesSent()));
```
##########
systemvm/debian/root/health_checks/cpu_usage_check.py:
##########
@@ -29,7 +29,7 @@ def main():
if "maxCpuUsage" not in data:
print("Missing maxCpuUsage in health_checks_data systemThresholds,
skipping")
- exit(0)
+ exit(2)
Review Comment:
The exit code should be 3 (UNKNOWN) instead of 2 (WARNING) when maxCpuUsage
configuration is missing, as this represents an unknown state rather than a
warning condition.
```suggestion
exit(3)
```
##########
framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java:
##########
@@ -378,23 +378,21 @@ protected T valueOf(String value) {
if (type.isAssignableFrom(Boolean.class)) {
return (T)Boolean.valueOf(value);
} else if (type.isAssignableFrom(Integer.class)) {
- return (T)new Integer(Integer.parseInt(value) *
multiplier.intValue());
+ return (T)Integer.valueOf(Integer.parseInt(value) *
multiplier.intValue());
} else if (type.isAssignableFrom(Long.class)) {
- return (T)new Long(Long.parseLong(value) * multiplier.longValue());
+ return (T)Long.valueOf(Long.parseLong(value) *
multiplier.longValue());
} else if (type.isAssignableFrom(Short.class)) {
- return (T)new Short(Short.parseShort(value));
+ return (T)Short.valueOf(Short.parseShort(value));
} else if (type.isAssignableFrom(String.class)) {
return (T)value;
Review Comment:
This condition for String.class is duplicated (appears on lines 386-387 and
392-393). The duplicate should be removed.
##########
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##########
@@ -1440,21 +1390,21 @@ private List<RouterHealthCheckResult>
parseHealthCheckResults(
return healthChecks;
}
- private List<RouterHealthCheckResult>
updateDbHealthChecksFromRouterResponse(final DomainRouterVO router, final
String monitoringResult) {
+ private void updateDbHealthChecksFromRouterResponse(final DomainRouterVO
router, final String monitoringResult) {
if (StringUtils.isBlank(monitoringResult)) {
logger.warn("Attempted parsing empty monitoring results string for
router {}", router);
- return Collections.emptyList();
+ return;
}
try {
logger.debug("Parsing and updating DB health check data for
router: {} with data: {}", router, monitoringResult);
final Type t = new TypeToken<Map<String, Map<String, Map<String,
String>>>>() {}.getType();
final Map<String, Map<String, Map<String, String>>> checks =
GsonHelper.getGson().fromJson(monitoringResult, t);
- return parseHealthCheckResults(checks, router);
+ parseHealthCheckResults(checks, router);
} catch (JsonSyntaxException ex) {
logger.error("Unable to parse the result of health checks due to "
+ ex.getLocalizedMessage(), ex);
}
- return Collections.emptyList();
+ return;
Review Comment:
This naked return statement is unnecessary. The method would naturally end
here without it.
```suggestion
```
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
