Maor Lipchuk has posted comments on this change. Change subject: core: added volume types existence validation ......................................................................
Patch Set 1: (3 comments) https://gerrit.ovirt.org/#/c/41967/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/CinderDisksValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/CinderDisksValidator.java: Line 92: // Specifying a volume type is not mandatory as Cinder can auto-select it. Line 93: continue; Line 94: } Line 95: OpenStackVolumeProviderProxy proxy = diskProxyMap.get(disk.getId()); Line 96: List<CinderVolumeType> volumeTypes = proxy.getVolumeTypes(); Sggestion: I would suggest to use CinderStorageRelatedDisksAndProxy private class (by calling getRelatedCinderDisksToStorageDomainMap method). Since every Storage Domain is related only to one proxy so it will make you avoid calling proxy.getVolumeTypes every disk. Line 97: boolean volumeTypeExists = CollectionUtils.exists(volumeTypes, new Predicate() { Line 98: @Override Line 99: public boolean evaluate(Object o) { Line 100: return ((CinderVolumeType) o).getName().equals(disk.getCinderVolumeType()); Line 98: @Override Line 99: public boolean evaluate(Object o) { Line 100: return ((CinderVolumeType) o).getName().equals(disk.getCinderVolumeType()); Line 101: } Line 102: }); Not sure I got this predicate. Why not simply use: volumeTypes.contains(disk.getCinderVolumeType()) ? Line 103: Line 104: if (!volumeTypeExists) { Line 105: return new ValidationResult(VdcBllMessages.CINDER_VOLUME_TYPE_NOT_EXISTS, Line 106: String.format("$cinderVolumeType %s", disk.getCinderVolumeType())); Line 103: Line 104: if (!volumeTypeExists) { Line 105: return new ValidationResult(VdcBllMessages.CINDER_VOLUME_TYPE_NOT_EXISTS, Line 106: String.format("$cinderVolumeType %s", disk.getCinderVolumeType())); Line 107: } the for loop here is not relevant since we already break it once an invalid disk's volume type is found. I would refactor this into two methods. The first is what is in the for loop, the function will get a disk and will return if it is valid or not. The second one will use the for loop for disks Line 108: } Line 109: return ValidationResult.VALID; Line 110: } Line 111: }); -- To view, visit https://gerrit.ovirt.org/41967 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I781ba3667f4f28db3ab79e2e31346154097ea664 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Maor Lipchuk <mlipc...@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
