Daniel Erez has posted comments on this change. Change subject: core: added volume types existence validation ......................................................................
Patch Set 2: (4 comments) https://gerrit.ovirt.org/#/c/41967/2/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 23: import org.ovirt.engine.core.dao.DiskDao; Line 24: import org.ovirt.engine.core.dao.StorageDomainDAO; Line 25: Line 26: import com.woorea.openstack.base.client.OpenStackResponseException; Line 27: import com.woorea.openstack.cinder.model.Limits; > Please separate to a different patch Done Line 28: Line 29: public class CinderDisksValidator { Line 30: Line 31: private Iterable<CinderDisk> cinderDisks; Line 105: Line 106: private boolean isLimitExceeded(Limits limits, VolumeClassification cinderType, int diskCount) { Line 107: if (cinderType == VolumeClassification.Snapshot) { Line 108: return (limits.getAbsolute().getTotalSnapshotsUsed() + diskCount >= limits.getAbsolute() Line 109: .getMaxTotalSnapshots()); > I assume it is a formatter? why is it related to the patch? Done Line 110: } Line 111: if (cinderType == VolumeClassification.Volume) { Line 112: return (limits.getAbsolute().getTotalVolumesUsed() + diskCount >= limits.getAbsolute().getMaxTotalVolumes()); Line 113: } Line 142: private OpenStackVolumeProviderProxy proxy; Line 143: Line 144: public CinderStorageRelatedDisksAndProxy(Guid storageDomainId, Line 145: List<CinderDisk> cinderDisks, Line 146: OpenStackVolumeProviderProxy proxy) { > not related to the patch Done Line 147: setStorageDomainId(storageDomainId); Line 148: setCinderDisks(cinderDisks); Line 149: setProxy(proxy); Line 150: } Line 193: public ValidationResult validateCinderVolumeTypesExist() { Line 194: return validate(new Callable<ValidationResult>() { Line 195: @Override Line 196: public ValidationResult call() { Line 197: final CinderDisk disk = cinderDisks.iterator().next(); > Why don't you use simply for loop as described in validateCinderDisksAlread since there's no real need to loop over disks in add disk flow, so better keep it simpler. Line 198: if (StringUtils.isEmpty(disk.getCinderVolumeType())) { Line 199: // Specifying a volume type is not mandatory as Cinder can auto-select it. Line 200: return ValidationResult.VALID; Line 201: } -- 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: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: 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
