Sergey Gotliv has posted comments on this change. Change subject: engine: Only iSCSI storage connections are viable for iSCSI Bond ......................................................................
Patch Set 2: (5 comments) http://gerrit.ovirt.org/#/c/23720/2//COMMIT_MSG Commit Message: Line 4: Commit: Sergey Gotliv <[email protected]> Line 5: CommitDate: 2014-01-30 13:31:49 +0200 Line 6: Line 7: engine: Only iSCSI storage connections are viable for iSCSI Bond Line 8: > Please add aditinal information such as: Done. Just pay attention that Query is renamed its not added. Line 9: Change-Id: Iae30db6aa0c5d2989e9e31566d6555d70259799e http://gerrit.ovirt.org/#/c/23720/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetConnectionsByDataCenterAndStorageTypeQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetConnectionsByDataCenterAndStorageTypeQuery.java: Line 11: } Line 12: Line 13: @Override Line 14: protected void executeQueryCommand() { Line 15: getQueryReturnValue().setReturnValue(getDbFacade().getStorageServerConnectionDao() > The if condition implemented in the DAO should be here, or in the helper cl I answered that in DAO. Line 16: .getConnectableStorageConnectionsByStorageType(getParameters().getId(), getParameters().getStorageType())); Line 17: } http://gerrit.ovirt.org/#/c/23720/2/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java: Line 64: } else if (storageType.isFileDomain()) { Line 65: storedProcedure = "GetConnectableFileStorageConnectionsByType"; Line 66: } else { Line 67: return Collections.emptyList(); Line 68: } > DAO should not contain this logic, this should be at the Helper class >DAO should not contain this logic, this should be at the Helper class This statement is too general. I believe that it depends on logic. Unfortunately, the way connections are implemented in the database requires to use 2 different stored procedures in order to fetch connection for FILE and BLOCK storage. But it doesn't mean I have to expose this fact to the external world. I would expect from DAO to hide such implementation detail. External world asking DAO "get...Connection...ByType" and doesn't know about the semantical differences between FILE and BLOCK. Line 69: Line 70: return getCallsHandler().executeReadList(storedProcedure, Line 71: mapper, Line 72: getCustomMapSqlParameterSource() http://gerrit.ovirt.org/#/c/23720/2/packaging/dbscripts/storages_san_sp.sql File packaging/dbscripts/storages_san_sp.sql: Line 554: Create or replace FUNCTION GetConnectableBlockStorageConnectionsByType(v_storage_pool_id UUID, v_storage_type integer) Line 555: RETURNS SETOF storage_server_connections STABLE Line 556: AS $procedure$ Line 557: BEGIN Line 558: RETURN QUERY SELECT distinct connection.* > why distinct? There One to Many relationship between Connection and LUNs, because Connection in the term of the Block storage domain is the TARGET. Since each target contains multiple luns without distinct this query will return the same connection multiple times. Please, test it. Line 559: FROM storage_server_connections connection, storage_domains domain, LUNs lun, LUN_storage_server_connection_map lun_connection Line 560: WHERE Line 561: connection.storage_type = v_storage_type and Line 562: connection.id = lun_connection.storage_server_connection and Line 571: Create or replace FUNCTION GetConnectableFileStorageConnectionsByType(v_storage_pool_id UUID, v_storage_type integer) Line 572: RETURNS SETOF storage_server_connections STABLE Line 573: AS $procedure$ Line 574: BEGIN Line 575: RETURN QUERY SELECT distinct connection.* > why distinct? I will remove it here. Line 576: FROM storage_server_connections connection, storage_domains domain Line 577: WHERE Line 578: connection.storage_type = v_storage_type and Line 579: connection.id = domain.storage and -- To view, visit http://gerrit.ovirt.org/23720 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae30db6aa0c5d2989e9e31566d6555d70259799e Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
