Allon Mureinik has posted comments on this change.
Change subject: core: introducing OvfAutoUpdate
......................................................................
Patch Set 18: (7 inline comments)
patchset 18: Answered Liron's comments inline.
....................................................
File backend/manager/dbscripts/vms_sp.sql
Line 720: AS $procedure$
Line 721: BEGIN
Line 722: RETURN QUERY SELECT vm.*
Line 723: FROM vms vm
Line 724: WHERE vm.vm_guid in (SELECT ID from
fnSplitterUuid(v_vms_ids));
SELECT fnSplitterUuid(v_vms_ids) should return one column too.
where did you see the (bad) pattern of select id from fnSplitter ?
Line 725: END; $procedure$
Line 726: LANGUAGE plpgsql;
Line 727:
Line 728:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java
Line 46: public class OvfDataUpdater {
Line 47: private static final Log log =
LogFactory.getLog(OvfDataUpdater.class);
Line 48: private static final OvfDataUpdater INSTANCE = new
OvfDataUpdater();
Line 49: private int ITEMS_COUNT_PER_UPDATE;
Line 50: protected static final int MAX_ITEMS_PER_SQL_STATEMENT = 100;
Consider renaming it - it's a bit confusing.
Line 51:
Line 52: private List<Guid> proccessedIdsInfo;
Line 53: private List<Long> proccessedOvfGenerationsInfo;
Line 54:
Line 180: if (verifyDisksNotLocked) {
Line 181: loadTemplateData(template);
Line 182: Long currentDbGeneration =
getVmStaticDao().getDbGeneration(template.getId());
Line 183: // equals() is used because currentDbGeneration
can be null in case that the template
Line 184: // was deleted during the run of OvfDataUpdater.
Using equals looks like a good practice to me, regardless of VMs being deleted.
Line 185: if
(template.getDb_generation().equals(currentDbGeneration)) {
Line 186: buildMetadataDictionaryForTemplate(template,
vmsAndTemplateMetadata);
Line 187: proccessedIdsInfo.add(template.getId());
Line 188:
proccessedOvfGenerationsInfo.add(template.getDb_generation());
Line 290: * loads additional need template data for it's ovf
Line 291: * @param template
Line 292: */
Line 293: protected void loadTemplateData(VmTemplate template) {
Line 294: if (template.getInterfaces() == null ||
template.getInterfaces().isEmpty()) {
sucks.
Line 295: template.setInterfaces(getVmNetworkInterfaceDao()
Line 296: .getAllForTemplate(template.getId()));
Line 297: }
Line 298: }
Line 301: * loads additional need vm data for it's ovf
Line 302: * @param vm
Line 303: */
Line 304: protected void loadVmData(VM vm) {
Line 305: if (vm.getInterfaces().isEmpty()) {
twelve rows above you have:
if (template.getInterfaces() == null || template.getInterfaces().isEmpty())
either the null check is redundant in the first own, or required here.
Line 306:
vm.setInterfaces(getVmNetworkInterfaceDao().getAllForVm(vm.getId()));
Line 307: }
Line 308: if (StringUtils.isEmpty(vm.getVmtName())) {
Line 309: if (!Guid.Empty.equals(vm.getVmtGuid())) {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/UpdateVMVDSCommandParameters.java
Line 4:
Line 5: import java.util.List;
Line 6: import java.util.HashMap;
Line 7: import java.util.Map;
Line 8:
I'm not arguing, I'm asking HOW it's relevant.
To me, it seems like all you did here was replacing FQCNs with imports
Line 9: public class UpdateVMVDSCommandParameters extends
StorageDomainIdParametersBase {
Line 10: public UpdateVMVDSCommandParameters(Guid storagePoolId,
Line 11: Map<Guid, KeyValuePairCompat<String, List<Guid>>>
infoDictionary) {
Line 12: super(storagePoolId);
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StoragePoolDAODbFacadeImpl.java
Line 192: return entity;
Line 193: }
Line 194: };
Line 195:
Line 196: return getCallsHandler().executeReadList("GetAllByStatus",
mapper, parameterSource);
I'd do both (add mapper + use it everywhere) in a separate patch, with this one
dependent on it.
Line 197: }
Line 198:
Line 199: @Override
Line 200: public List<storage_pool> getAll(Guid userID, boolean isFiltered)
{
--
To view, visit http://gerrit.ovirt.org/9328
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b5132300fb1f1fd94f771cab15efe5246dbeca8
Gerrit-PatchSet: 18
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: liron aravot <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
