Juan Hernandez has posted comments on this change. Change subject: restapi: OpenStack image provider ......................................................................
Patch Set 6: (3 comments) http://gerrit.ovirt.org/#/c/33975/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/openstack/BackendOpenStackImageProviderHelper.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/openstack/BackendOpenStackImageProviderHelper.java: Line 37: */ Line 38: public static Guid getStorageDomainId(BackendResource resource, String providerId) { Line 39: // The backend doesn't have any mechanism to obtain the images other than listing the images provided by the Line 40: // storage domain that is created for the provider, and the only way to find that provider is to iterate the Line 41: // complete list. This is potentially very slow, so it should be improved in the future. > I think you can use 'StorageDomainDAO -> getAllByConnectionId' (confusing n There is no query corresponding to getAllByConnectionId, and the RESTAPI should not invoke directly DAO methods. The user doesn't provide any storage domain identifier, he provides the identifier of the provider. Line 42: Guid storageDomainId = null; Line 43: List<StorageDomain> storageDomains = Line 44: resource.runQuery(VdcQueryType.GetAllStorageDomains, new VdcQueryParametersBase()).getReturnValue(); Line 45: for (StorageDomain storageDomain : storageDomains) { http://gerrit.ovirt.org/#/c/33975/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/openstack/BackendOpenStackImageProviderResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/openstack/BackendOpenStackImageProviderResource.java: Line 71: } Line 72: Line 73: @Override Line 74: public Response testConnectivity(Action action) { Line 75: Provider provider = BackendExternalProviderHelper.getProvider(this, id); > Can we extract this code to an upper level? It should be generic for all pr I prefer to avoid the additional cryptic generic constructs that this will introduce. I will move the code to the helper class. Line 76: ProviderParameters parameters = new ProviderParameters(provider); Line 77: return performAction(VdcActionType.TestProviderConnectivity, parameters); Line 78: } Line 79: http://gerrit.ovirt.org/#/c/33975/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/openstack/BackendOpenStackImageResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/openstack/BackendOpenStackImageResource.java: Line 61: Line 62: @Override Line 63: public Response doImport(Action action) { Line 64: validateParameters(action, "storageDomain.id|name"); Line 65: Guid storageDomainId = BackendOpenStackImageProviderHelper.getStorageDomainId(this, providerId); > can't we use the specified storage domain id instead of fetching it? The storage domain id given by the caller is the target, whilst we are fetching the source, which isn't given by the caller. Line 66: ImportRepoImageParameters parameters = new ImportRepoImageParameters(); Line 67: parameters.setSourceRepoImageId(id); Line 68: parameters.setSourceStorageDomainId(storageDomainId); Line 69: parameters.setStoragePoolId(getDataCenterId(getStorageDomainId(action))); -- To view, visit http://gerrit.ovirt.org/33975 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I77fca2208b18d54f9177ec8b0178768a6b815f20 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@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
