Omer Frenkel has posted comments on this change. Change subject: core: add timestamp for vm-dynamic updates ......................................................................
Patch Set 2: (4 comments) @Piotr, in the new patch im using System.nanoTime() which is monotonic and should be safe to use for time comparison https://gerrit.ovirt.org/#/c/41520/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ManagingVmCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ManagingVmCommand.java: Line 15: protected void executeVDSCommand() { Line 16: vmManager.lock(); Line 17: try { Line 18: executeVmCommand(); Line 19: vmManager.setLastUpdateDate(Calendar.getInstance().getTime()); > Note that a side effect of putting it here will be that every time the user which should be ok because the race condition will cause the new value to be deleted, and this is what we want to fix i will move the call to a method that could be overridden if needed. Line 20: } finally { Line 21: vmManager.unlock(); Line 22: } Line 23: } https://gerrit.ovirt.org/#/c/41520/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java: Line 286: new VmsMonitoring(this, Line 287: fetcher.getChangedVms(), Line 288: fetcher.getVmsWithChangedDevices(), Line 289: auditLogDirector, Line 290: fetcher.getFetchTime() > don't you prefer to take the time here before calling the 'fetch' method in Done Line 291: ).perform(); Line 292: } Line 293: } Line 294: https://gerrit.ovirt.org/#/c/41520/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsMonitoring.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsMonitoring.java: Line 169: Line 170: private void unlockVmsManager() { Line 171: Date updateTime = Calendar.getInstance().getTime(); Line 172: for (VmManager vmManager : vmManagers.values()) { Line 173: vmManager.setLastUpdateDate(updateTime); > why is this part needed? there will be no additional cycle of VmsMonitoring just in case this will be used for other stuff than this class, i prefer to keep this value correct Line 174: vmManager.unlock(); Line 175: } Line 176: } Line 177: https://gerrit.ovirt.org/#/c/41520/2/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/VmsMonitoringTest.java File backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/VmsMonitoringTest.java: Line 33: import org.ovirt.engine.core.utils.MockEJBStrategyRule; Line 34: import org.ovirt.engine.core.vdsbroker.vdsbroker.entities.VmInternalData; Line 35: Line 36: import static org.mockito.Matchers.any; Line 37: import static org.mockito.Mockito.when; > do you have the right formater set? i think so Line 38: Line 39: @Ignore Line 40: @RunWith(MockitoJUnitRunner.class) Line 41: /** -- To view, visit https://gerrit.ovirt.org/41520 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If858993450cf957babdcf337e9b7867a282657bc Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
