Liron Aravot has posted comments on this change. Change subject: core: Add a query for attached Storage Domains ......................................................................
Patch Set 16: (3 comments) http://gerrit.ovirt.org/#/c/34233/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsWithAttachedStoragePoolGuidQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsWithAttachedStoragePoolGuidQuery.java: Line 69: private boolean isFileDomain() { Line 70: return getParameters().getStorageServerConnection() != null; Line 71: } Line 72: Line 73: private List<StorageDomainStatic> getBlockStorageDomains() { > no, it is the same scenario as we do when we add a new block domain, it sho the query shouldn't be aware that it's executed by the UI, tomorrow it can be executed from other command. regardless, currently you have one behavior for for file (where you disconnect) and for block. Line 74: VDSReturnValue vdsReturnValue = null; Line 75: List<StorageDomain> storageDomains = getParameters().getStorageDomainList(); Line 76: List<StorageDomainStatic> storageDomainsWithAttachedStoragePoolId = new ArrayList<>(); Line 77: Line 88: } catch (RuntimeException e) { Line 89: logErrorMessage(storageDomain); Line 90: continue; Line 91: } Line 92: if (!vdsReturnValue.getSucceeded()) { > there is no different behaviour ? in one you return null and in one you continue. Line 93: logErrorMessage(storageDomain); Line 94: return null; Line 95: } Line 96: Line 128: } Line 129: } Line 130: Line 131: getBackend().runInternalAction(VdcActionType.DisconnectStorageServerConnection, Line 132: new StorageServerConnectionParametersBase(getParameters().getStorageServerConnection(), vdsId)); > 1. I prefer that it will be more implicit, I've already verified this with 1. in some actions you use the storagehelper and in some you are not, we should use the storage helper in all flows as it should contain the right logic, no one will remember to add changes here if it will be necessary for this connection which is error prone. 2. it should be passed by a parameter as the query may be used by others as well. Line 133: } Line 134: return storageDomains; Line 135: } Line 136: -- To view, visit http://gerrit.ovirt.org/34233 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6fc7a2d722611b1bccecee9868223ff437596ed Gerrit-PatchSet: 16 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
