Copilot commented on code in PR #12398:
URL: https://github.com/apache/cloudstack/pull/12398#discussion_r2681053169
##########
server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java:
##########
@@ -144,51 +145,66 @@ public void processConnect(Host host, StartupCommand cmd,
boolean forRebalance)
}
@Override
- public synchronized boolean processDisconnect(long agentId, Status state) {
+ public boolean processDisconnect(long agentId, Status state) {
return processDisconnect(agentId, null, null, state);
}
@Override
- public synchronized boolean processDisconnect(long agentId, String uuid,
String name, Status state) {
+ public boolean processDisconnect(long agentId, String uuid, String name,
Status state) {
+ logger.debug("Starting disconnect for Agent [id: {}, uuid: {}, name:
{}]", agentId, uuid, name);
Host host = _storageManager.getHost(agentId);
if (host == null) {
logger.warn("Agent [id: {}, uuid: {}, name: {}] not found, not
disconnecting pools", agentId, uuid, name);
return false;
}
if (host.getType() != Host.Type.Routing) {
+ logger.debug("Host [id: {}, uuid: {}, name: {}] is not of type {},
skip", agentId, uuid, name, Host.Type.Routing);
return false;
}
+ logger.debug("Looking for connected Storage Pools for Host [id: {},
uuid: {}, name: {}]", agentId, uuid, name);
List<StoragePoolHostVO> storagePoolHosts =
_storageManager.findStoragePoolsConnectedToHost(host.getId());
if (storagePoolHosts == null) {
- if (logger.isTraceEnabled()) {
- logger.trace("No pools to disconnect for host: {}", host);
- }
+ logger.debug("No pools to disconnect for host: {}", host);
return true;
}
+ logger.debug("Found {} pools to disconnect for host: {}",
storagePoolHosts.size(), host);
boolean disconnectResult = true;
- for (StoragePoolHostVO storagePoolHost : storagePoolHosts) {
+ int storagePoolHostsSize = storagePoolHosts.size();
+ for (int i = 0; i < storagePoolHostsSize; i++) {
+ StoragePoolHostVO storagePoolHost = storagePoolHosts.get(i);
+ logger.debug("Processing disconnect from Storage Pool {} ({} of
{}) for host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
StoragePoolVO pool =
_poolDao.findById(storagePoolHost.getPoolId());
if (pool == null) {
+ logger.debug("No Storage Pool found with id {} ({} of {}) for
host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
continue;
}
if (!pool.isShared()) {
+ logger.debug("Storage Pool {} ({}) ({} of {}) is not shared
for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, host);
continue;
}
// Handle only PowerFlex pool for now, not to impact other pools
behavior
if (pool.getPoolType() != StoragePoolType.PowerFlex) {
+ logger.debug("Storage Pool {} ({}) ({} of {}) is not of type
{} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, pool.getPoolType(), host);
continue;
}
+ logger.debug("Sending disconnect to Storage Pool {} ({}) ({} of
{}) for host: {}", pool.getName(), pool.getUuid(), i, storagePoolHostsSize,
host);
+ Profiler disconnectProfiler = new Profiler();
try {
+ disconnectProfiler.start();
_storageManager.disconnectHostFromSharedPool(host, pool);
} catch (Exception e) {
logger.error("Unable to disconnect host {} from storage pool
{} due to {}", host, pool, e.toString());
disconnectResult = false;
+ } finally {
+ disconnectProfiler.stop();
+ long disconnectDuration =
disconnectProfiler.getDurationInMillis() / 1000;
+ logger.debug("Finished disconnect with result {} from Storage
Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult,
pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host,
disconnectDuration);
Review Comment:
The profiling duration calculation uses integer division which will truncate
to 0 for operations taking less than 1000 milliseconds. This makes the timing
information less useful since most disconnect operations likely complete in
under a second. Consider using getDurationInMillis() directly and logging in
milliseconds, or convert to a double/float for fractional seconds.
```suggestion
long disconnectDurationMillis =
disconnectProfiler.getDurationInMillis();
logger.debug("Finished disconnect with result {} from
Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} ms",
disconnectResult, pool.getName(), pool.getUuid(), i, storagePoolHostsSize,
host, disconnectDurationMillis);
```
##########
server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java:
##########
@@ -144,51 +145,66 @@ public void processConnect(Host host, StartupCommand cmd,
boolean forRebalance)
}
@Override
- public synchronized boolean processDisconnect(long agentId, Status state) {
+ public boolean processDisconnect(long agentId, Status state) {
return processDisconnect(agentId, null, null, state);
}
@Override
- public synchronized boolean processDisconnect(long agentId, String uuid,
String name, Status state) {
+ public boolean processDisconnect(long agentId, String uuid, String name,
Status state) {
+ logger.debug("Starting disconnect for Agent [id: {}, uuid: {}, name:
{}]", agentId, uuid, name);
Host host = _storageManager.getHost(agentId);
if (host == null) {
logger.warn("Agent [id: {}, uuid: {}, name: {}] not found, not
disconnecting pools", agentId, uuid, name);
return false;
}
if (host.getType() != Host.Type.Routing) {
+ logger.debug("Host [id: {}, uuid: {}, name: {}] is not of type {},
skip", agentId, uuid, name, Host.Type.Routing);
return false;
}
+ logger.debug("Looking for connected Storage Pools for Host [id: {},
uuid: {}, name: {}]", agentId, uuid, name);
List<StoragePoolHostVO> storagePoolHosts =
_storageManager.findStoragePoolsConnectedToHost(host.getId());
if (storagePoolHosts == null) {
- if (logger.isTraceEnabled()) {
- logger.trace("No pools to disconnect for host: {}", host);
- }
+ logger.debug("No pools to disconnect for host: {}", host);
return true;
}
+ logger.debug("Found {} pools to disconnect for host: {}",
storagePoolHosts.size(), host);
boolean disconnectResult = true;
- for (StoragePoolHostVO storagePoolHost : storagePoolHosts) {
+ int storagePoolHostsSize = storagePoolHosts.size();
+ for (int i = 0; i < storagePoolHostsSize; i++) {
+ StoragePoolHostVO storagePoolHost = storagePoolHosts.get(i);
+ logger.debug("Processing disconnect from Storage Pool {} ({} of
{}) for host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
StoragePoolVO pool =
_poolDao.findById(storagePoolHost.getPoolId());
if (pool == null) {
+ logger.debug("No Storage Pool found with id {} ({} of {}) for
host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
continue;
}
if (!pool.isShared()) {
+ logger.debug("Storage Pool {} ({}) ({} of {}) is not shared
for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, host);
continue;
}
// Handle only PowerFlex pool for now, not to impact other pools
behavior
if (pool.getPoolType() != StoragePoolType.PowerFlex) {
+ logger.debug("Storage Pool {} ({}) ({} of {}) is not of type
{} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, pool.getPoolType(), host);
Review Comment:
The log message has an incorrect argument order. The message template shows
"Storage Pool {} ({}) ({} of {}) is not of type {} for host: {}", but the
arguments provided are: pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, pool.getPoolType(), host. The fifth placeholder expects
the pool type that the pool is NOT, but pool.getPoolType() returns the actual
type of the pool. The expected type (StoragePoolType.PowerFlex) should be
included in the log message instead.
```suggestion
logger.debug("Storage Pool {} ({}) ({} of {}) is not of type
{} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, StoragePoolType.PowerFlex, host);
```
##########
server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java:
##########
@@ -144,51 +145,66 @@ public void processConnect(Host host, StartupCommand cmd,
boolean forRebalance)
}
@Override
- public synchronized boolean processDisconnect(long agentId, Status state) {
+ public boolean processDisconnect(long agentId, Status state) {
return processDisconnect(agentId, null, null, state);
}
@Override
- public synchronized boolean processDisconnect(long agentId, String uuid,
String name, Status state) {
+ public boolean processDisconnect(long agentId, String uuid, String name,
Status state) {
+ logger.debug("Starting disconnect for Agent [id: {}, uuid: {}, name:
{}]", agentId, uuid, name);
Host host = _storageManager.getHost(agentId);
if (host == null) {
logger.warn("Agent [id: {}, uuid: {}, name: {}] not found, not
disconnecting pools", agentId, uuid, name);
return false;
}
if (host.getType() != Host.Type.Routing) {
+ logger.debug("Host [id: {}, uuid: {}, name: {}] is not of type {},
skip", agentId, uuid, name, Host.Type.Routing);
return false;
}
+ logger.debug("Looking for connected Storage Pools for Host [id: {},
uuid: {}, name: {}]", agentId, uuid, name);
List<StoragePoolHostVO> storagePoolHosts =
_storageManager.findStoragePoolsConnectedToHost(host.getId());
if (storagePoolHosts == null) {
- if (logger.isTraceEnabled()) {
- logger.trace("No pools to disconnect for host: {}", host);
- }
+ logger.debug("No pools to disconnect for host: {}", host);
return true;
}
+ logger.debug("Found {} pools to disconnect for host: {}",
storagePoolHosts.size(), host);
boolean disconnectResult = true;
- for (StoragePoolHostVO storagePoolHost : storagePoolHosts) {
+ int storagePoolHostsSize = storagePoolHosts.size();
+ for (int i = 0; i < storagePoolHostsSize; i++) {
+ StoragePoolHostVO storagePoolHost = storagePoolHosts.get(i);
+ logger.debug("Processing disconnect from Storage Pool {} ({} of
{}) for host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
StoragePoolVO pool =
_poolDao.findById(storagePoolHost.getPoolId());
if (pool == null) {
+ logger.debug("No Storage Pool found with id {} ({} of {}) for
host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
continue;
}
if (!pool.isShared()) {
+ logger.debug("Storage Pool {} ({}) ({} of {}) is not shared
for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, host);
continue;
}
// Handle only PowerFlex pool for now, not to impact other pools
behavior
if (pool.getPoolType() != StoragePoolType.PowerFlex) {
+ logger.debug("Storage Pool {} ({}) ({} of {}) is not of type
{} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, pool.getPoolType(), host);
continue;
}
+ logger.debug("Sending disconnect to Storage Pool {} ({}) ({} of
{}) for host: {}", pool.getName(), pool.getUuid(), i, storagePoolHostsSize,
host);
+ Profiler disconnectProfiler = new Profiler();
try {
+ disconnectProfiler.start();
_storageManager.disconnectHostFromSharedPool(host, pool);
} catch (Exception e) {
logger.error("Unable to disconnect host {} from storage pool
{} due to {}", host, pool, e.toString());
disconnectResult = false;
+ } finally {
+ disconnectProfiler.stop();
+ long disconnectDuration =
disconnectProfiler.getDurationInMillis() / 1000;
+ logger.debug("Finished disconnect with result {} from Storage
Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult,
pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host,
disconnectDuration);
}
Review Comment:
The disconnectResult variable logged in the finally block at line 207 does
not accurately represent the result of the current disconnect operation. The
variable is set to false only when an exception occurs, but it retains its
previous value (from previous iterations or the initial true value) when the
current operation succeeds. This makes the log message misleading. Consider
using a local boolean variable to track the result of the current disconnect
operation specifically.
```suggestion
boolean currentDisconnectResult = true;
try {
disconnectProfiler.start();
_storageManager.disconnectHostFromSharedPool(host, pool);
} catch (Exception e) {
logger.error("Unable to disconnect host {} from storage pool
{} due to {}", host, pool, e.toString());
currentDisconnectResult = false;
} finally {
disconnectProfiler.stop();
long disconnectDuration =
disconnectProfiler.getDurationInMillis() / 1000;
logger.debug("Finished disconnect with result {} from
Storage Pool {} ({}) ({} of {}) for host: {}, duration: {} secs",
currentDisconnectResult, pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, host, disconnectDuration);
}
if (!currentDisconnectResult) {
disconnectResult = false;
}
```
##########
server/src/main/java/com/cloud/storage/listener/StoragePoolMonitor.java:
##########
@@ -144,51 +145,66 @@ public void processConnect(Host host, StartupCommand cmd,
boolean forRebalance)
}
@Override
- public synchronized boolean processDisconnect(long agentId, Status state) {
+ public boolean processDisconnect(long agentId, Status state) {
return processDisconnect(agentId, null, null, state);
}
@Override
- public synchronized boolean processDisconnect(long agentId, String uuid,
String name, Status state) {
+ public boolean processDisconnect(long agentId, String uuid, String name,
Status state) {
+ logger.debug("Starting disconnect for Agent [id: {}, uuid: {}, name:
{}]", agentId, uuid, name);
Host host = _storageManager.getHost(agentId);
if (host == null) {
logger.warn("Agent [id: {}, uuid: {}, name: {}] not found, not
disconnecting pools", agentId, uuid, name);
return false;
}
if (host.getType() != Host.Type.Routing) {
+ logger.debug("Host [id: {}, uuid: {}, name: {}] is not of type {},
skip", agentId, uuid, name, Host.Type.Routing);
return false;
}
+ logger.debug("Looking for connected Storage Pools for Host [id: {},
uuid: {}, name: {}]", agentId, uuid, name);
List<StoragePoolHostVO> storagePoolHosts =
_storageManager.findStoragePoolsConnectedToHost(host.getId());
if (storagePoolHosts == null) {
- if (logger.isTraceEnabled()) {
- logger.trace("No pools to disconnect for host: {}", host);
- }
+ logger.debug("No pools to disconnect for host: {}", host);
return true;
}
+ logger.debug("Found {} pools to disconnect for host: {}",
storagePoolHosts.size(), host);
boolean disconnectResult = true;
- for (StoragePoolHostVO storagePoolHost : storagePoolHosts) {
+ int storagePoolHostsSize = storagePoolHosts.size();
+ for (int i = 0; i < storagePoolHostsSize; i++) {
+ StoragePoolHostVO storagePoolHost = storagePoolHosts.get(i);
+ logger.debug("Processing disconnect from Storage Pool {} ({} of
{}) for host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
StoragePoolVO pool =
_poolDao.findById(storagePoolHost.getPoolId());
if (pool == null) {
+ logger.debug("No Storage Pool found with id {} ({} of {}) for
host: {}", storagePoolHost.getPoolId(), i, storagePoolHostsSize, host);
continue;
}
if (!pool.isShared()) {
+ logger.debug("Storage Pool {} ({}) ({} of {}) is not shared
for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, host);
continue;
}
// Handle only PowerFlex pool for now, not to impact other pools
behavior
if (pool.getPoolType() != StoragePoolType.PowerFlex) {
+ logger.debug("Storage Pool {} ({}) ({} of {}) is not of type
{} for host: {}, ignore disconnect", pool.getName(), pool.getUuid(), i,
storagePoolHostsSize, pool.getPoolType(), host);
continue;
}
+ logger.debug("Sending disconnect to Storage Pool {} ({}) ({} of
{}) for host: {}", pool.getName(), pool.getUuid(), i, storagePoolHostsSize,
host);
+ Profiler disconnectProfiler = new Profiler();
try {
+ disconnectProfiler.start();
_storageManager.disconnectHostFromSharedPool(host, pool);
} catch (Exception e) {
logger.error("Unable to disconnect host {} from storage pool
{} due to {}", host, pool, e.toString());
disconnectResult = false;
+ } finally {
+ disconnectProfiler.stop();
+ long disconnectDuration =
disconnectProfiler.getDurationInMillis() / 1000;
+ logger.debug("Finished disconnect with result {} from Storage
Pool {} ({}) ({} of {}) for host: {}, duration: {} secs", disconnectResult,
pool.getName(), pool.getUuid(), i, storagePoolHostsSize, host,
disconnectDuration);
Review Comment:
The loop index message format is inconsistent. The message shows "i of
storagePoolHostsSize" which displays as "0 of 5", "1 of 5", etc. This is
confusing because the index is 0-based while the total count is 1-based.
Consider using "i+1" to display "1 of 5", "2 of 5" for better readability in
log messages.
--
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]
