Moti Asayag has posted comments on this change.
Change subject: core: Update VM devices on DB when hash changes
......................................................................
Patch Set 1: (15 inline comments)
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java
Line 765: if
(!VmDeviceCommonUtils.isOldClusterVersion(_vds.getvds_group_compatibility_version()))
{
At this point the treatment is made for VM in down status. With the VDSM return
it in case the hash is dirty ? Is it relevant if the VM is down ?
Line 795: for (java.util.Map.Entry<VmDynamic, VmStatistics> vm_helper :
_runningVms.values()) {
* no need for java.util.prefix doesn't required
* user proper java naming convention vm_helper --> vmHelper
Line 797: String dbHash = _vmDict.get(vm.getId()).getHash();
I'd verify the return value isn't null before taking further action for that
VM, since there is a possibility the VM was removed from backend.
Line 801: vm.setHash(vm.getHash());
since there is a chance the VM was already modified and pushed to
_vmDynamicToSave by AddVmDynamicToList(vm) from other place in the flow, i'd
get its value from there and update, else add it.
Line 825: private XmlRpcStruct[] getVmInfo(List<String> vmsToUpdate) {
you should wrap this call, since a specific failure will cause all of the
refresh to fail.
Line 845: int i = 0;
please remove the i, it isn't being used.
Line 857: log.infoFormat("Recieved a Device without an address
when processing VM devices, skipping device: {0}.",
Does the inner map suppose to contain the VM ID as well ? Else, i'd add it for
the context of this message.
Line 866:
DbFacade.getInstance().getVmDeviceDAO().update(vmDevice);
I think that it will be better adding it to a list that will be updated at
once using updateAllInTransaction() as being called from SaveDataToDb()
Line 883: if (device.getIsPlugged()) {
please add vm id for the log messages in this method
Line 888:
DbFacade.getInstance().getVmDeviceDAO().update(device);
same comment as before, cache the objects to be modified and update all at once.
Line 892:
DbFacade.getInstance().getVmDeviceDAO().remove(device.getId());
same for those - perform deleteAll from same update point of code in a single
transaction.
Line 903: */
Please use constant values from VdsProperties
Line 912: Object o = device.getItem("specParams");
why not use VdsProperties.SpecParams instead "specParams" ?
Line 915: specParams = ((Map<String, String>) o).toString();
this looks very error prone, there is implementation of the specific map class
and if the concrete class will be some you don't expect - the structure of the
specParams might be unsupported format.
Line 924: DbFacade.getInstance().getVmDeviceDAO().save(newDevice);
Same comment at before - cache them and save them.
--
To view, visit http://gerrit.ovirt.org/2410
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia963db83d88cb11c57be197c40d64f1e1b3c88c9
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
