Maor Lipchuk has posted comments on this change.
Change subject: core: retrieve correct domains status in updateConn
......................................................................
Patch Set 4: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 151: }
Line 152: }
Line 153: }
Line 154: Guid storageDomainId = lun.getStorageDomainId();
Line 155: if (storageDomainId != null) {
You don't have to check here for null value, at the worst part
getAllForStorageDomain will return you an empty list
Line 156: StoragePool pool = null;
Line 157: StorageDomain domain = null;
Line 158: List<StoragePool> storagePools =
getStoragePoolDAO().getAllForStorageDomain(storageDomainId);
Line 159: if (!storagePools.isEmpty()) {
Line 153: }
Line 154: Guid storageDomainId = lun.getStorageDomainId();
Line 155: if (storageDomainId != null) {
Line 156: StoragePool pool = null;
Line 157: StorageDomain domain = null;
If pool amd domain is being used only in the if (!storagePools.isEmpty())
condition I would prefer that they will be delared inside the if context (a bit
more readable IMHO)
Line 158: List<StoragePool> storagePools =
getStoragePoolDAO().getAllForStorageDomain(storageDomainId);
Line 159: if (!storagePools.isEmpty()) {
Line 160: pool = storagePools.get(0);
Line 161: domain =
getStorageDomainDao().getForStoragePool(storageDomainId,pool.getId());
Line 158: List<StoragePool> storagePools =
getStoragePoolDAO().getAllForStorageDomain(storageDomainId);
Line 159: if (!storagePools.isEmpty()) {
Line 160: pool = storagePools.get(0);
Line 161: domain =
getStorageDomainDao().getForStoragePool(storageDomainId,pool.getId());
Line 162: if
(!(domain.getStatus().equals(StorageDomainStatus.Maintenance))) {
it is better to use StorageDomainStatus.Maintenance.equals(domain.getStatus())
(I know that domain could not be null but still it is a better practice IMHO)
Line 163: String domainName =
domain.getStorageName();
Line 164: problematicDomainNames.add(domainName);
Line 165: }
Line 166: else {
Line 162: if
(!(domain.getStatus().equals(StorageDomainStatus.Maintenance))) {
Line 163: String domainName =
domain.getStorageName();
Line 164: problematicDomainNames.add(domainName);
Line 165: }
Line 166: else {
formatting, should be '} else {'
Line 167: domains.add(domain);
Line 168: }
Line 169: }
Line 170: else {
Line 167: domains.add(domain);
Line 168: }
Line 169: }
Line 170: else {
Line 171: //we cannot retrieve domain's status , thus
it's better not to update the connection
Suggestion: I would add a log here, so we can have better understanding when we
will try to read the log in case of a bug.
Line 172: String domainName =
lun.getStorageDomainName();
Line 173: problematicDomainNames.add(domainName);
Line 174: }
Line 175:
--
To view, visit http://gerrit.ovirt.org/17070
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I19221ef1f32dd7117e020cf6750034d64b4b47db
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
