Moti Asayag has posted comments on this change. Change subject: new feature: Vm Payload ......................................................................
Patch Set 12: (15 inline comments) .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java Line 315: getParameters().getVmStaticData().getVmPayload().setContent(Base64.encodeBase64String( Note that we're sending the SysPrep content for 3.0 as a byte array which is supported type by the XmlRpc spec (http://ws.apache.org/xmlrpc/types.html) See ovirt-engine/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/CreateVmFromSysPrepVDSCommand.java Perhaps it is worth being aligned on the format sent to agent across versions? .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmPayloadQuery.java Line 23: VmDeviceDAO dao = DbFacade.getInstance().getVmDeviceDAO(); Please replace with: VmDeviceDAO dao = getDbFacade().getVmDeviceDAO(); it will ease mocking when writing tests. Line 24: List<VmDevice> disks = new ArrayList(dao.getVmDeviceByVmIdAndType(getParameters().getId(), VmDeviceType.DISK.getName())); 1. The type of the ArrayList is missing: List<VmDevice> disks = new ArrayList<VmDevice>... 2. You don't need here the ArrayList at all - just use: List<VmDevice> disks = dao.getVmDeviceByVmIdAndType(getParameters().getId(), VmDeviceType.DISK.getName()); If no value returns, the dao returns an empty list. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java Line 96: VmDeviceDAO dao = DbFacade.getInstance().getVmDeviceDAO(); please replace with: VmDeviceDAO dao = getVmDeviceDao(); Line 100: List<VmDevice> disks = new ArrayList(dao.getVmDeviceByVmIdAndType(getVmId(), VmDeviceType.DISK.getName())); no need for the new ArratList. Line 110: List devs = new ArrayList<VmDeviceId>(); Please add type to the List - List<VmDeviceId> or consider replacing the content of this block with dao.removeAll(Collections.singletonList(oldPayload.getId())); which won't require typing. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java Line 526: Integer lengthInKb = 2 * Config.<Integer> GetValue(ConfigValues.PayloadSize) / 1024; maybe constant for the 1024 (avoid magic numbers) .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmPayload.java Line 11: public class VmPayload implements Serializable { please add serialVersionUID since this class is Serializable Line 15: please move the static members to the beginning of the class. Line 18: private static String SpecParamsParamSeparator = ","; this property and the below are note being used. Do we need those ? Line 44: this.content = files.get(key).toString(); since there is a single value, perhaps it would be better to add a break after the first assignment ? Line 49: return specParams.containsKey(SpecParamsPayload); please use java naming convention, starting a method name with a lower case: isPayload(...) .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java Line 1111: @TypeConverterAttribute(Integer.class) please move the property beneath the property with ordinal 352 .................................................... File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java Line 113: vm.getVmPayload().getSpecParams(), this is a potential NullPointerException since you checked previously that vm.getVmPayload() might be null if reached here. Line 180: vm.getVmPayload().getSpecParams(), same comment for potential NPE, according to the condition vm.getVmPayload() might be null. -- To view, visit http://gerrit.ovirt.org/3243 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c2820acfa8ec0f736e5fe0ce192e84df0915f12 Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Yaniv Kaul <yk...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
