Michael Kublin has posted comments on this change.
Change subject: core: Extract SD validations from ImagesHandler
......................................................................
Patch Set 9: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/MultipleStorageDomainsValidator.java
Line 31: /**
Line 32: * Constructor from Guids
Line 33: * @param sdIds A {@link Collection} of storage domain IDs to be
validated
Line 34: */
Line 35: public MultipleStorageDomainsValidator(NGuid spId,
Collection<Guid> sdIds) {
this is unneeded, or remove it or change name to stroragePoolId in constructor
Line 36: this.storagePoolId = spId;
Line 37: domainValidators = new HashMap<Guid,
StorageDomainValidator>(sdIds.size());
Line 38: for (Guid id : sdIds) {
Line 39: domainValidators.put(id, null);
Line 33: * @param sdIds A {@link Collection} of storage domain IDs to be
validated
Line 34: */
Line 35: public MultipleStorageDomainsValidator(NGuid spId,
Collection<Guid> sdIds) {
Line 36: this.storagePoolId = spId;
Line 37: domainValidators = new HashMap<Guid,
StorageDomainValidator>(sdIds.size());
Allon, please read java doc for HashMap, how it works and about it capacity,
after that please fix a code
Line 38: for (Guid id : sdIds) {
Line 39: domainValidators.put(id, null);
Line 40: }
Line 41: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java
Line 137: if (!spUpResult.isValid()) {
Line 138:
message.add(spUpResult.getMessage().name());
Line 139: retValue = false;
Line 140: }
Line 141:
This part of code actually should be written inside some method
Line 142: if (retValue && (!vm.isAutoStartup() ||
!runParams.getIsInternal())) {
Line 143: Set<Guid> storageDomainIds =
ImagesHandler.getAllStorageIdsForImageIds(vmImages);
Line 144: MultipleStorageDomainsValidator
storageDomainValidator =
Line 145: new
MultipleStorageDomainsValidator(vm.getStoragePoolId(), storageDomainIds);
Line 217: }
Line 218: }
Line 219: return retValue;
Line 220: }
Line 221:
Allon, why I need this method it used only once. Why you can not use single
line validate(storageDomainValidator.allDomainsExistAndActive(), message) ???
I don't understand this style of coding - when one line of code replaced by
method.
It is obvious that it is not efficient code styling, and it not improving
readability of code.
Line 222: protected boolean
validateAllDomainsUp(MultipleStorageDomainsValidator storageDomainValidator,
List<String> message) {
Line 223: return
validate(storageDomainValidator.allDomainsExistAndActive(), message);
Line 224: }
Line 225:
Line 279: }
Line 280:
Line 281: /**
Line 282: * Check isValid, storageDomain and diskSpace only if VM is not
HA VM
Line 283: */
Java doc not changed?
Line 284: protected boolean performImageChecksForRunningVm
Line 285: (VM vm, List<String> message, RunVmParams runParams,
List<DiskImage> vmDisks) {
Line 286: return ImagesHandler.PerformImagesChecks(message,
Line 287: vm.getStoragePoolId(),
--
To view, visit http://gerrit.ovirt.org/12249
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cfc895dceed4de520476ed5e3fa9c1c7cfd78f4
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@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: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
