Daniel Erez has posted comments on this change. Change subject: core: validate disks existence on create snapshot ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/23706/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java: Line 505: validate(vmValidator.vmNotSavingRestoring()); Line 506: } Line 507: Line 508: private boolean isSpecifiedDisksExist(List<DiskImage> disks) { Line 509: if (disks != null) { > if the list is empty it's the same, same comment about the indentation here I prefer to keep this check so the method could 'protect' it self. I can just return true if disks is null for indentation sake.. Line 510: List<DiskImage> disksNotExistInDb = ImagesHandler.imagesSubtract(getParameters().getDisks(), getDisksList()); Line 511: if (!disksNotExistInDb.isEmpty()) { Line 512: List<String> disksNotExistInDbIds = new ArrayList<>(disksNotExistInDb.size()); Line 513: for (DiskImage diskImage : disksNotExistInDb) { Line 507: Line 508: private boolean isSpecifiedDisksExist(List<DiskImage> disks) { Line 509: if (disks != null) { Line 510: List<DiskImage> disksNotExistInDb = ImagesHandler.imagesSubtract(getParameters().getDisks(), getDisksList()); Line 511: if (!disksNotExistInDb.isEmpty()) { > perhaps change to Agree, done. Line 512: List<String> disksNotExistInDbIds = new ArrayList<>(disksNotExistInDb.size()); Line 513: for (DiskImage diskImage : disksNotExistInDb) { Line 514: disksNotExistInDbIds.add(diskImage.getId().toString()); Line 515: } Line 508: private boolean isSpecifiedDisksExist(List<DiskImage> disks) { Line 509: if (disks != null) { Line 510: List<DiskImage> disksNotExistInDb = ImagesHandler.imagesSubtract(getParameters().getDisks(), getDisksList()); Line 511: if (!disksNotExistInDb.isEmpty()) { Line 512: List<String> disksNotExistInDbIds = new ArrayList<>(disksNotExistInDb.size()); > there's no need to specify the size here, as the capacity grows autmaticall Since the size is already known, It should be better performance-wise to specify the size of the array - as the array grows by 150 percent on every addition of a member. Line 513: for (DiskImage diskImage : disksNotExistInDb) { Line 514: disksNotExistInDbIds.add(diskImage.getId().toString()); Line 515: } Line 516: Line 514: disksNotExistInDbIds.add(diskImage.getId().toString()); Line 515: } Line 516: Line 517: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_NOT_EXIST, Line 518: String.format("$%1$s %2$s", "diskIds", StringUtils.join(disksNotExistInDbIds, ","))); > perhaps we can have this check in DiskValidator so we could re-use it. Hmm, yeah, though about it... Wasn't sure as I would need to create the validator twice here (as I need to check a different set of lists each time). I'll reconsider. Line 519: } Line 520: } Line 521: Line 522: return true; -- To view, visit http://gerrit.ovirt.org/23706 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d75ad139d048f83b08d9289e43d909b29f89695 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
