Copilot commented on code in PR #14343:
URL: https://github.com/apache/cloudstack/pull/14343#discussion_r4347273924
##########
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java:
##########
@@ -1037,7 +1038,7 @@ public Map<Long, VirtualMachine.PowerState>
updatePowerState(
return notUpdated;
}
StringBuilder sql = new StringBuilder("UPDATE `cloud`.`vm_instance`
SET " +
- "`power_host` = ?, `power_state_update_time` = now(),
`power_state` = CASE ");
+ "`power_host` = ?, `power_state_update_time` = ?,
`power_state` = CASE ");
Review Comment:
The PR title/description says timestamps should be updated to the timezone
the MS is running in, but the implementation explicitly binds the timestamp
using GMT (`DateUtil.currentGMTTime()` + `gmtCalendar()`). If the intent is to
store MS-local time, this should use the MS/default timezone calendar instead
(or omit the explicit GMT calendar). If the intent is to store UTC/GMT
consistently, please update the PR title/description to match the behavior to
avoid confusion for reviewers and future maintainers.
##########
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java:
##########
@@ -1052,6 +1053,7 @@ public Map<Long, VirtualMachine.PowerState>
updatePowerState(
TransactionLegacy txn = TransactionLegacy.currentTxn();
try (PreparedStatement pstmt =
txn.prepareAutoCloseStatement(sql.toString())) {
pstmt.setLong(1, powerHostId);
+ pstmt.setTimestamp(2, new
Timestamp(DateUtil.currentGMTTime().getTime()), gmtCalendar());
Review Comment:
The PR title/description says timestamps should be updated to the timezone
the MS is running in, but the implementation explicitly binds the timestamp
using GMT (`DateUtil.currentGMTTime()` + `gmtCalendar()`). If the intent is to
store MS-local time, this should use the MS/default timezone calendar instead
(or omit the explicit GMT calendar). If the intent is to store UTC/GMT
consistently, please update the PR title/description to match the behavior to
avoid confusion for reviewers and future maintainers.
##########
engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java:
##########
@@ -1052,6 +1053,7 @@ public Map<Long, VirtualMachine.PowerState>
updatePowerState(
TransactionLegacy txn = TransactionLegacy.currentTxn();
try (PreparedStatement pstmt =
txn.prepareAutoCloseStatement(sql.toString())) {
pstmt.setLong(1, powerHostId);
+ pstmt.setTimestamp(2, new
Timestamp(DateUtil.currentGMTTime().getTime()), gmtCalendar());
Review Comment:
This line applies two separate UTC/GMT mechanisms at once: it uses
`DateUtil.currentGMTTime()` and also passes a GMT `Calendar` to `setTimestamp`.
Since `setTimestamp(..., Calendar)` is already the mechanism that controls
timezone interpretation on the JDBC side, consider simplifying to a single,
unambiguous source of truth (e.g., use the current instant from
`System.currentTimeMillis()`/`new Date()` and rely on the GMT calendar for UTC
binding). This reduces the risk of accidental double-adjustment depending on
`DateUtil.currentGMTTime()` semantics.
##########
engine/schema/src/test/java/com/cloud/vm/dao/VMInstanceDaoImplTest.java:
##########
@@ -210,6 +220,51 @@ public void
testUpdatePowerStateNoChangeMaxUpdatesInvalidStateVmRunning() {
assertTrue(result);
}
+ @Test
+ public void testUpdatePowerStatesBindsGmtUpdateTimeInsteadOfDbNow() throws
Exception {
+ TimeZone defaultTimeZone = TimeZone.getDefault();
+ try {
+ for (String timeZone : List.of("UTC", "Europe/Amsterdam",
"America/New_York", "Asia/Kolkata", "Pacific/Auckland")) {
+ TimeZone.setDefault(TimeZone.getTimeZone(timeZone));
+ verifyBulkUpdatePowerStatesBindsGmtUpdateTime(timeZone);
+ }
+ } finally {
+ TimeZone.setDefault(defaultTimeZone);
+ }
Review Comment:
This test mutates the JVM-wide default timezone (`TimeZone.setDefault`),
which can cause flaky behavior if the test suite (or other tests in the same
JVM) runs concurrently/parallel. A safer approach is to avoid changing global
state and instead assert only on the bound `Calendar`/prepared-statement
interaction (or use a test framework mechanism that prevents parallel execution
for this test/class when global state must be changed).
--
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]
