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

Reply via email to