Allon Mureinik has posted comments on this change.
Change subject: restapi: add storage server connections resource
......................................................................
Patch Set 22: (8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageServerConnectionByIdQuery.java
Line 13: @Override
Line 14: protected void executeQueryCommand() {
Line 15: getQueryReturnValue().setReturnValue(DbFacade.getInstance().
Line 16: getStorageServerConnectionDao().get(getParameters().
Line 17: getServerConnectionId()));
I'd do this in a separate patchset.
This one is crowded as is :-)
Line 18: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
Line 40: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_NOT_EXIST);
Line 41: }
Line 42: // if user passed only the connection id for removal, vdsm
still needs few more details in order to disconnect, so
Line 43: // bringing them from db and repopulating them in the
connection object received in input parameters
Line 44: populateMissingFields(connection);
can you clarify where these are used? I could see any references
(not saying you're wrong, just trying to understand the flow here.)
Line 45: StorageType storageType = connection.getstorage_type();
Line 46: if (storageType.isFileDomain()) {
Line 47: // go to storage domain static, get all storage domains
where storage field = storage connection id
Line 48: domains = getStorageDomainsByConnId(connectionId);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/StorageServerConnectionQueryParametersBase.java
Line 18: public StorageServerConnectionQueryParametersBase(String
serverConnectionId) {
Line 19: setServerConnectionId(serverConnectionId);
Line 20: }
Line 21:
Line 22: //used by REST because AbstractBackendResource has id member that
is always assumed to be guid
consider changing the comment to a javadoc comment (/** */ instead of //)
Line 23: public StorageServerConnectionQueryParametersBase(Guid
serverConnectionId) {
Line 24: this(serverConnectionId.toString());
Line 25: }
Line 26:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java
Line 22: StorageServerConnectionDAO {
Line 23:
Line 24: @Override
Line 25: public List<StorageServerConnections> getAll() {
Line 26: MapSqlParameterSource parameterSource =
getCustomMapSqlParameterSource();
I'd inline this
Line 27: return
getCallsHandler().executeReadList("GetAllstorage_server_connections",
mapper,parameterSource) ;
Line 28: }
Line 29:
Line 30: @Override
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageServerConnectionResource.java
Line 28: }
Line 29:
Line 30: @Override
Line 31: public Storage update(Storage connection) {
Line 32: validateParameters(connection, "address", "type", "host");
isn't address required only for iSCSI?
Line 33: validateEnums(Storage.class, connection);
Line 34: return performUpdate(connection,
Line 35: new
QueryIdResolver<String>(VdcQueryType.GetStorageServerConnectionById,
StorageServerConnectionQueryParametersBase.class),
Line 36:
VdcActionType.UpdateStorageServerConnection,
Line 57: }
Line 58:
Line 59: private Guid getHostId(Host host) {
Line 60: // presence of host ID or name already validated
Line 61: return host.isSetId()
The indentation is off for these two statements
Line 62: ? new Guid(host.getId())
Line 63: : host.isSetName()
Line 64: ? getEntity(VDS.class,
Line 65: SearchType.VDS,
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageServerConnectionsResource.java
Line 65: break;
Line 66: case NFS:
Line 67: validateParameters(storage, "path");
Line 68: break;
Line 69: case LOCALFS:
why have separate cases for NFS and LOCALFS?
Line 70: validateParameters(storage, "path");
Line 71: break;
Line 72: case POSIXFS:
Line 73: case GLUSTERFS:
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/StorageDomainMapper.java
Line 12: import org.ovirt.engine.api.restapi.model.StorageFormat;
Line 13: import org.ovirt.engine.api.restapi.utils.GuidUtils;
Line 14: import
org.ovirt.engine.core.common.businessentities.StorageDomainStatic;
Line 15: import
org.ovirt.engine.core.common.businessentities.StorageServerConnections;
Line 16:
General: I'd do all the formatting changes in a different patch.
It adds a lot of noise to this one.
Line 17: public class StorageDomainMapper {
Line 18:
Line 19: @Mapping(from = StorageDomain.class, to =
StorageDomainStatic.class)
Line 20: public static StorageDomainStatic map(StorageDomain model,
StorageDomainStatic template) {
--
To view, visit http://gerrit.ovirt.org/16617
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If6bc32ead098390723825872f6fb292097d52835
Gerrit-PatchSet: 22
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: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
