GutoVeronezi commented on a change in pull request #4575:
URL: https://github.com/apache/cloudstack/pull/4575#discussion_r661714750
##########
File path:
server/src/main/java/com/cloud/network/element/VirtualRouterElement.java
##########
@@ -411,12 +411,12 @@ public boolean stopVpn(final RemoteAccessVpn vpn) throws
ResourceUnavailableExce
if (canHandle(network, Service.Vpn)) {
final List<DomainRouterVO> routers =
_routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (routers == null || routers.isEmpty()) {
- s_logger.debug("Virtual router elemnt doesn't need stop vpn on
the backend; virtual router doesn't " + "exist in the network " +
network.getId());
+ s_logger.debug(String.format("Virtual router element doesn't
need stop VPN on the backend; virtual router doesn't exist in the network %s",
network.getId()));
Review comment:
We could simplify this message, e.g.:
`There is no virtual router in network [%s], it is not necessary to stop the
VPN on backend.`
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -4489,8 +4495,8 @@ private void orchestrateMigrateForScale(final String
vmUuid, final long srcHostI
s_logger.info("Migration was unsuccessful. Cleaning up: " +
vm);
_alertMgr.sendAlert(alertType, fromHost.getDataCenterId(),
fromHost.getPodId(),
- "Unable to migrate vm " + vm.getInstanceName() + "
from host " + fromHost.getName() + " in zone " + dest.getDataCenter().getName()
+ " and pod " +
- dest.getPod().getName(), "Migrate Command
failed. Please check logs.");
+ "Unable to migrate vm " + vm.getInstanceName() + "
from " + fromHost + " in zone " + dest.getDataCenter().getName() + " and pod " +
Review comment:
We could use `String.format` here.
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -4474,13 +4480,13 @@ private void orchestrateMigrateForScale(final String
vmUuid, final long srcHostI
try {
_agentMgr.send(srcHostId, new
Commands(cleanup(vm.getInstanceName())), null);
} catch (final AgentUnavailableException e) {
- s_logger.error("AgentUnavailableException while
cleanup on source host: " + srcHostId);
+
s_logger.error(String.format("AgentUnavailableException while cleanup on source
%s: ", srcHost), e);
Review comment:
As `AgentUnavailableException` will appears in stacktrace, we could
improve this message with something more clean, e.g `Unable to cleanup source
%s...`.
##########
File path: server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
##########
@@ -2481,7 +2482,7 @@ public void deleteRoutingHost(final HostVO host, final
boolean isForced, final b
try {
_vmMgr.destroy(vm.getUuid(), false);
} catch (final Exception e) {
- final String errorMsg = "There was an error Destory the
vm: " + vm + " as a part of hostDelete id=" + host.getId();
+ String errorMsg = String.format("There was an error when
destroying the VM: %s as a part of hostDelete: %s", vm, host);
Review comment:
Could you to take a look into the VM's `toString` implementation?
https://github.com/apache/cloudstack/blob/7835c0812062381c7e5dd5ab9d719297cd3187dc/engine/schema/src/main/java/com/cloud/vm/VMInstanceVO.java#L508-L510
Seems like there will be duplicated words:
`There was an error when destroying the VM: VM Instance...`
It would apply to every time we log the VM.
##########
File path: server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
##########
@@ -2607,13 +2608,13 @@ protected void connectAndRestartAgentOnHost(HostVO
host, String username, String
final com.trilead.ssh2.Connection connection =
SSHCmdHelper.acquireAuthorizedConnection(
host.getPrivateIpAddress(), 22, username, password);
if (connection == null) {
- throw new CloudRuntimeException("SSH to agent is enabled, but
failed to connect to host: " + host.getPrivateIpAddress());
+ throw new CloudRuntimeException(String.format("SSH to agent is
enabled, but failed to connect to %s via IP address %s.", host,
host.getPrivateIpAddress()));
Review comment:
I would rather use `[]` as var delimiter in some cases; IMHO, it is
easier to recognize what is a variable and what is fixed in the final message,
e.g.:
`SSH to agent is enabled, but failed to connect to %s via IP address [%s].`.
##########
File path:
server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java
##########
@@ -571,7 +571,7 @@ private void performPreFlightChecks(List<Host> hosts, int
timeout, String payloa
for (Host host : hosts) {
Ternary<Boolean, String, Boolean> result =
performStageOnHost(host, Stage.PreFlight, timeout, payload, forced);
if (result.third() &&
!hostsToAvoidMaintenance.containsKey(host.getId())) {
- s_logger.debug("Host " + host.getId() + " added to the avoid
maintenance set");
+ s_logger.debug(String.format("%s added to the avoid
maintenance set", host));
hostsToAvoidMaintenance.put(host.getId(), "Pre-flight stage
set to avoid maintenance");
}
Review comment:
This code seems repeated, but changing parameters; We could extract it
to a method.
##########
File path:
server/src/main/java/com/cloud/resource/RollingMaintenanceManagerImpl.java
##########
@@ -439,7 +439,7 @@ private void enableClusterIfDisabled(Cluster cluster,
Set<Long> disabledClusters
*/
private boolean isMaintenanceStageAvoided(Host host, Map<Long, String>
hostsToAvoidMaintenance, List<HostSkipped> hostsSkipped) {
if (hostsToAvoidMaintenance.containsKey(host.getId())) {
- s_logger.debug("Host " + host.getId() + " is not being put into
maintenance, skipping it");
+ s_logger.debug(String.format("%s is not being put into
maintenance, skipping it", host));
Review comment:
I think this message could be more clear, e.g.:
`%s is in avoid maintenance list [%s], skipping its maintenance...`
--
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]
