Copilot commented on code in PR #10710:
URL: https://github.com/apache/cloudstack/pull/10710#discussion_r2313459688
##########
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##########
@@ -1364,9 +1310,9 @@ private void updateRouterHealthCheckResult(final long
routerId, String checkName
}
private RouterHealthCheckResultVO parseHealthCheckVOFromJson(final long
routerId,
- final String checkName, final String checkType, final Map<String,
String> checkData,
- final Map<String, Map<String, RouterHealthCheckResultVO>>
checksInDb) {
- boolean success = Boolean.parseBoolean(checkData.get("success"));
+ final String
checkName, final String checkType, final Map<String, String> checkData,
+ final
Map<String, Map<String, RouterHealthCheckResultVO>> checksInDb) {
+ RouterHealthStatus success =
RouterHealthStatus.valueOf(checkData.get("success"));
Review Comment:
Using valueOf() directly without validation could throw
IllegalArgumentException if the string value doesn't match any enum constant.
Consider adding error handling or using a safer parsing method to handle
invalid health status values gracefully.
```suggestion
RouterHealthStatus success =
parseRouterHealthStatus(checkData.get("success"));
```
##########
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##########
@@ -2343,10 +2277,7 @@ public boolean finalizeCommandsOnStart(final Commands
cmds, final VirtualMachine
// restart network if restartNetwork = false is not specified in
profile
// parameters
- boolean reprogramGuestNtwks = true;
- if (profile.getParameter(Param.ReProgramGuestNetworks) != null &&
(Boolean) profile.getParameter(Param.ReProgramGuestNetworks) == false) {
- reprogramGuestNtwks = false;
- }
+ boolean reprogramGuestNtwks = !
Boolean.FALSE.equals(profile.getParameter(Param.ReProgramGuestNetworks));
Review Comment:
The spacing around the '!' operator is inconsistent with Java conventions.
It should be `boolean reprogramGuestNtwks =
!Boolean.FALSE.equals(profile.getParameter(Param.ReProgramGuestNetworks));` (no
space after the exclamation mark).
```suggestion
boolean reprogramGuestNtwks =
!Boolean.FALSE.equals(profile.getParameter(Param.ReProgramGuestNetworks));
```
##########
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##########
@@ -3259,18 +3178,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 logging parameters are inconsistent - line 3191 uses
answerFinal.getBytesReceived() but line 3192 uses
stats.getCurrentBytesReceived(), but the log message suggests both should show
the same type of data for comparison.
##########
server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java:
##########
@@ -545,19 +540,11 @@ public boolean configure(final String name, final
Map<String, Object> params) th
final Map<String, String> configs =
_configDao.getConfiguration("AgentManager", params);
- _routerRamSize = NumbersUtil.parseInt(configs.get("router.ram.size"),
DEFAULT_ROUTER_VM_RAMSIZE);
- _routerCpuMHz = NumbersUtil.parseInt(configs.get("router.cpu.mhz"),
DEFAULT_ROUTER_CPU_MHZ);
+ int _routerRamSize =
NumbersUtil.parseInt(configs.get("router.ram.size"), DEFAULT_ROUTER_VM_RAMSIZE);
+ int _routerCpuMHz =
NumbersUtil.parseInt(configs.get("router.cpu.mhz"), DEFAULT_ROUTER_CPU_MHZ);
Review Comment:
Variables _routerRamSize and _routerCpuMHz are declared as local variables
but use instance variable naming convention (underscore prefix). They should be
renamed to routerRamSize and routerCpuMHz to follow local variable naming
conventions.
```suggestion
int routerRamSize =
NumbersUtil.parseInt(configs.get("router.ram.size"), DEFAULT_ROUTER_VM_RAMSIZE);
int routerCpuMHz =
NumbersUtil.parseInt(configs.get("router.cpu.mhz"), DEFAULT_ROUTER_CPU_MHZ);
```
--
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]
