Allon Mureinik has posted comments on this change.
Change subject: restapi - #755579: Return proper http status for errors
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(24 inline comments)
+3 on the concept, but the implementation has some issues:
1. You neglected to move
/backend/manager/modules/dal/src/main/resources/bundles to common - These
translation files should be coupled to the VdcBllMessgaes enum you moved.
2. Same goes for
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/DuplicateKeyTest
3. Also note the inline comments - might as well fix 'em if you're revisiting
this patch, but IMHO, they alone do not warrant a (-1) review.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java
Line 36: import org.ovirt.engine.core.common.businessentities.VolumeFormat;
Line 37: import
org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface;
Line 38: import org.ovirt.engine.core.common.errors.VdcBLLException;
Line 39: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 40: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly and group all the common.errors imports together :-)
Line 41: import org.ovirt.engine.core.common.queries.DiskImageList;
Line 42: import
org.ovirt.engine.core.common.queries.GetAllFromExportDomainQueryParameters;
Line 43: import org.ovirt.engine.core.common.queries.VdcQueryReturnValue;
Line 44: import org.ovirt.engine.core.common.queries.VdcQueryType;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java
Line 17: import org.ovirt.engine.core.common.businessentities.DiskImage;
Line 18: import org.ovirt.engine.core.common.businessentities.StorageDomainType;
Line 19: import org.ovirt.engine.core.common.errors.VdcBLLException;
Line 20: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 21: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly and group all the common.errors imports together :-)
Line 22: import org.ovirt.engine.core.common.utils.Pair;
Line 23: import org.ovirt.engine.core.compat.Guid;
Line 24: import org.ovirt.engine.core.compat.KeyValuePairCompat;
Line 25: import org.ovirt.engine.core.utils.transaction.TransactionMethod;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterCommandBase.java
Line 21: import org.ovirt.engine.core.common.businessentities.VdsStatic;
Line 22: import
org.ovirt.engine.core.common.businessentities.gluster.GlusterBrickEntity;
Line 23: import org.ovirt.engine.core.common.constants.gluster.GlusterConstants;
Line 24: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 25: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly "errors" should come before "locks" :-)
Line 26: import org.ovirt.engine.core.common.utils.Pair;
Line 27: import org.ovirt.engine.core.dao.VdsStaticDAO;
Line 28: import org.ovirt.engine.core.dao.gluster.GlusterBrickDao;
Line 29:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java
Line 33: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 34: import org.ovirt.engine.core.common.utils.Pair;
Line 35: import org.ovirt.engine.core.compat.Guid;
Line 36: import org.ovirt.engine.core.dao.DiskDao;
Line 37: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly and group all the common imports together :-)
Line 38: import org.ovirt.engine.core.dao.DiskImageDAO;
Line 39: import org.ovirt.engine.core.dao.StorageDomainDAO;
Line 40: import org.ovirt.engine.core.utils.collections.MultiValueMapUtils;
Line 41:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/SetupNetworksHelper.java
Line 17: import org.ovirt.engine.core.common.businessentities.network.Network;
Line 18: import
org.ovirt.engine.core.common.businessentities.network.NetworkBootProtocol;
Line 19: import
org.ovirt.engine.core.common.businessentities.network.VdsNetworkInterface;
Line 20: import org.ovirt.engine.core.common.utils.ValidationUtils;
Line 21: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly "errors" should come before "utils":-)
Line 22: import org.ovirt.engine.core.dal.dbbroker.DbFacade;
Line 23: import org.ovirt.engine.core.utils.NetworkUtils;
Line 24:
Line 25: public class SetupNetworksHelper {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java
Line 33: import
org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotType;
Line 34: import org.ovirt.engine.core.common.errors.VdcBLLException;
Line 35: import org.ovirt.engine.core.common.errors.VdcBllErrors;
Line 36: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 37: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly and group all the common.errors imports together :-)
Line 38: import org.ovirt.engine.core.common.utils.Pair;
Line 39: import org.ovirt.engine.core.compat.Guid;
Line 40: import org.ovirt.engine.core.compat.NGuid;
Line 41: import org.ovirt.engine.core.dao.SnapshotDao;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStoragePoolWithStoragesCommand.java
Line 23: import org.ovirt.engine.core.common.businessentities.StoragePoolIsoMap;
Line 24: import org.ovirt.engine.core.common.errors.VdcBLLException;
Line 25: import org.ovirt.engine.core.common.errors.VdcBllErrors;
Line 26: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 27: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly and group all the common imports together :-)
Line 28: import org.ovirt.engine.core.common.utils.Pair;
Line 29: import
org.ovirt.engine.core.common.vdscommands.CreateStoragePoolVDSCommandParameters;
Line 30: import org.ovirt.engine.core.common.vdscommands.VDSCommandType;
Line 31: import org.ovirt.engine.core.common.vdscommands.VDSReturnValue;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
Line 15: import org.ovirt.engine.core.common.errors.VdcBllErrors;
Line 16: import org.ovirt.engine.core.common.errors.VdcFault;
Line 17: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 18: import org.ovirt.engine.core.common.utils.Pair;
Line 19: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly and group all the common imports together :-)
Line 20: import org.ovirt.engine.core.common.validation.NfsMountPointConstraint;
Line 21: import org.ovirt.engine.core.common.validation.group.CreateEntity;
Line 22: import org.ovirt.engine.core.compat.Guid;
Line 23: import org.ovirt.engine.core.dal.dbbroker.DbFacade;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainCommand.java
Line 17: import org.ovirt.engine.core.common.businessentities.VDS;
Line 18: import org.ovirt.engine.core.common.businessentities.StorageDomain;
Line 19: import org.ovirt.engine.core.common.businessentities.StoragePoolIsoMap;
Line 20: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 21: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly - "errors" should come before "locks":-)
Line 22: import org.ovirt.engine.core.common.utils.Pair;
Line 23: import
org.ovirt.engine.core.common.vdscommands.DeactivateStorageDomainVDSCommandParameters;
Line 24: import
org.ovirt.engine.core.common.vdscommands.DisconnectStoragePoolVDSCommandParameters;
Line 25: import
org.ovirt.engine.core.common.vdscommands.IrsBaseVDSCommandParameters;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/FenceVdsManualyCommand.java
Line 24: import org.ovirt.engine.core.common.businessentities.VDSStatus;
Line 25: import org.ovirt.engine.core.common.businessentities.VdsSpmStatus;
Line 26: import org.ovirt.engine.core.common.businessentities.StoragePool;
Line 27: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 28: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly - "errors" should come before "locks" :-)
Line 29: import org.ovirt.engine.core.common.utils.Pair;
Line 30: import
org.ovirt.engine.core.common.vdscommands.FenceSpmStorageVDSCommandParameters;
Line 31: import
org.ovirt.engine.core.common.vdscommands.ResetIrsVDSCommandParameters;
Line 32: import
org.ovirt.engine.core.common.vdscommands.SpmStatusVDSCommandParameters;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ForceRemoveStorageDomainCommand.java
Line 13: import
org.ovirt.engine.core.common.businessentities.StorageDomainSharedStatus;
Line 14: import
org.ovirt.engine.core.common.businessentities.StorageDomainStatus;
Line 15: import org.ovirt.engine.core.common.businessentities.StorageDomainType;
Line 16: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 17: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly - "errors" should come before "locks" :-)
Line 18: import org.ovirt.engine.core.common.utils.Pair;
Line 19: import
org.ovirt.engine.core.common.vdscommands.DetachStorageDomainVDSCommandParameters;
Line 20: import
org.ovirt.engine.core.common.vdscommands.IrsBaseVDSCommandParameters;
Line 21: import org.ovirt.engine.core.common.vdscommands.VDSCommandType;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStoragePoolCommand.java
Line 28: import org.ovirt.engine.core.common.businessentities.StoragePool;
Line 29: import org.ovirt.engine.core.common.businessentities.network.Network;
Line 30: import org.ovirt.engine.core.common.errors.VdcBLLException;
Line 31: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 32: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly - "errors" should come before "locks" :-)
Line 33: import org.ovirt.engine.core.common.utils.Pair;
Line 34: import
org.ovirt.engine.core.common.vdscommands.FormatStorageDomainVDSCommandParameters;
Line 35: import
org.ovirt.engine.core.common.vdscommands.IrsBaseVDSCommandParameters;
Line 36: import org.ovirt.engine.core.common.vdscommands.VDSCommandType;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Line 27: import org.ovirt.engine.core.common.businessentities.VmStatic;
Line 28: import org.ovirt.engine.core.common.businessentities.network.Network;
Line 29: import
org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface;
Line 30: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 31: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly "errors: should come before "locks" :-)
Line 32: import
org.ovirt.engine.core.common.queries.IsVmWithSameNameExistParameters;
Line 33: import org.ovirt.engine.core.common.queries.VdcQueryType;
Line 34: import org.ovirt.engine.core.common.utils.Pair;
Line 35: import org.ovirt.engine.core.common.utils.VmDeviceType;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 26: import
org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface;
Line 27: import org.ovirt.engine.core.common.locks.LockingGroup;
Line 28: import org.ovirt.engine.core.common.utils.Pair;
Line 29: import org.ovirt.engine.core.compat.Guid;
Line 30: import org.ovirt.engine.core.common.errors.VdcBllMessages;
If you went to the trouble of organizing the imports, might as well do it
correctly and group all the common imports together :-)
Line 31: import org.ovirt.engine.core.utils.transaction.TransactionMethod;
Line 32: import org.ovirt.engine.core.utils.transaction.TransactionSupport;
Line 33:
Line 34: @NonTransactiveCommandAttribute
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/ErrorType.java
Line 1: package org.ovirt.engine.core.common.errors;
Line 2:
Line 3: public enum ErrorType {
Line 4:
Line 5: /*
Java docs start with /**
Line 6: * Client passed invalid parameters in the request. For example - a
necessary parameter is missing (null), bad ID
Line 7: * supplied (e.g: moving disk to a different storage-domain - ID of
the storage-domain is wrong or malformed), some
Line 8: * combination of parameters is not supported, a list which is
expected to include at least one element is empty,
Line 9: * client tried to update a non-updatable property, and things
like: an export domain was given where a data domain
Line 10: * is expected, etc. This request would never be legal.
Line 11: */
Line 12: BAD_PARAMETERS,
Line 13:
Line 14: /*
Java docs start with /**
Line 15: * Client made a request which is not applicable in the current
status of the engine. The same request, under
Line 16: * different circumstances, would be applicable. For example, a
client wants to start a VM which is already running.
Line 17: * In other circumstances (VM down) this request would be
applicable.
Line 18: */
Line 17: * In other circumstances (VM down) this request would be
applicable.
Line 18: */
Line 19: CONFLICT,
Line 20:
Line 21: /*
Java docs start with /**
Line 22: * Client parameters directly violate a constraint. For example,
setting VM memory of 500GB would violate the
Line 23: * constraint of maximum VM memory (even if the maximum is
configurable in Vdc_Options table, we consider it a
Line 24: * constraint violation). This is different from CONFLICT, because
(taking vdc_options as a given) under no
Line 25: * circumstances would it be ok to give a VM 500GB memory.
Line 29: INCOMPATIBLE_VERSION,
Line 30:
Line 31: INTERNAL_ERROR,
Line 32:
Line 33: NOT_SUPPORTED,
no javadoc for these four?
Line 34:
Line 35: /*
Line 36: * Client failed to authenticate.
Line 37: */
Line 31: INTERNAL_ERROR,
Line 32:
Line 33: NOT_SUPPORTED,
Line 34:
Line 35: /*
Java docs start with /**
Line 36: * Client failed to authenticate.
Line 37: */
Line 38: NO_AUTHENTICATION,
Line 39:
Line 36: * Client failed to authenticate.
Line 37: */
Line 38: NO_AUTHENTICATION,
Line 39:
Line 40: /*
Java docs start with /**
Line 41: * Client lacks permission to perform this operation (after
authentication).
Line 42: */
Line 43: NO_PERMISSION,
Line 44:
Line 41: * Client lacks permission to perform this operation (after
authentication).
Line 42: */
Line 43: NO_PERMISSION,
Line 44:
Line 45: /*
Java docs start with /**
Line 46: * An illegal state in the server does not enable the request to
be executed. This means internal data corruption
Line 47: * had occured.
Line 48: */
Line 49: DATA_CORRUPTION,
....................................................
Commit Message
Line 5: CommitDate: 2013-05-13 10:58:14 +0300
Line 6:
Line 7: restapi - #755579: Return proper http status for errors
Line 8:
Line 9: Until now all engine failures resulted in 400 BAD_REQEST.
Infrasctructure was added to return proper error
Long line, please try to stick to <=70 chars
Line 10: messages, depending on the exact failure.
Line 11:
Line 12: Bug-Url: https://bugzilla.redhat.com/755579
Line 13:
Line 7: restapi - #755579: Return proper http status for errors
Line 8:
Line 9: Until now all engine failures resulted in 400 BAD_REQEST.
Infrasctructure was added to return proper error
Line 10: messages, depending on the exact failure.
Line 11:
>From our face2face I understand that the bulk of this patch is moving
>VdcBllMessages from dal to common and classifying them.
I don't see this explained here - probably not a bad idea to add it.
Line 12: Bug-Url: https://bugzilla.redhat.com/755579
Line 13:
Line 14: Change-Id: I395e4b303eda174360ece709ddd7de6b1e3ce93f
Line 9: Until now all engine failures resulted in 400 BAD_REQEST.
Infrasctructure was added to return proper error
Line 10: messages, depending on the exact failure.
Line 11:
Line 12: Bug-Url: https://bugzilla.redhat.com/755579
Line 13:
Please remove the blank line here
Line 14: Change-Id: I395e4b303eda174360ece709ddd7de6b1e3ce93f
--
To view, visit http://gerrit.ovirt.org/14498
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I395e4b303eda174360ece709ddd7de6b1e3ce93f
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ori Liel <ol...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Ori Liel <ol...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
