Moti Asayag has posted comments on this change.
Change subject: core: Add QoS to to DB and entities
......................................................................
Patch Set 8: (12 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/AddNetworkQoSCommand.java
Line 29:
Line 30: @Override
Line 31: protected void setActionMessageParameters() {
Line 32: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__ADD);
Line 33: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__NETWORK_QOS);
this line can be pulled into setActionMessageParameters() method in the
NetworkQoSCommandBase class since it repeats in any of the NetworkQoS commands.
Line 34: }
Line 35:
Line 36: @Override
Line 37: public AuditLogType getAuditLogTypeValue() {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/NetworkQoSCommandBase.java
Line 18: private NetworkQoS networkQoS;
Line 19:
Line 20: public NetworkQoSCommandBase(NetworkQoSParametersBase parameters) {
Line 21: super(parameters);
Line 22:
setStoragePoolId(parameters.getNetworkQoS().getStoragePoolId());
please note that the c'tor is being invoked prior to any other method,
including the candoaction validations, therefore there is a chance a client
might send incorrect values, e.g not providing any NetworkQoS entity, so the
could should be NPE safe here.
Line 23: addCustomValue("NetworkQoSName",
parameters.getNetworkQoS().getName());
Line 24: getParameters().setShouldBeLogged(true);
Line 25: }
Line 26:
Line 26:
Line 27: public NetworkQoS getNetworkQoS() {
Line 28: if (networkQoS == null) {
Line 29: if (getParameters().getNetworkQoS() == null) {
Line 30: if (getParameters().getNetworkQoSGuid() != null) {
why there is a need to expect either networkQoS or just its ID ? I think we can
settle only for the entity itself, with no need to provide the ID as a
parameter.
This will reduce the logic of this method and will simplify it to be a simple
getter.
Line 31:
getNetworkQoSDao().get(getParameters().getNetworkQoSGuid());
Line 32: }
Line 33: } else {
Line 34: networkQoS = getParameters().getNetworkQoS();
Line 37: return networkQoS;
Line 38: }
Line 39:
Line 40: protected boolean validateNameAndStoragePoolNotNull() {
Line 41: if (getNetworkQoS().getName() == null ||
getNetworkQoS().getStoragePoolId() == null) {
i assume the NetworkQoS name should have also constrains for length and
permitted characters. The proper way would be using @Size and @Pattern
annotations and also the @NotNull for the relevant entities from NetworkQoS
class.
Line 42: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_QOS_MISSING_DATA);
Line 43: }
Line 44: return true;
Line 45: }
Line 81: getNetworkQoS().getOutboundPeak(),
Line 82: getNetworkQoS().getOutboundBurst());
Line 83: }
Line 84:
Line 85: private boolean missingValue(Integer average, Integer peak,
Integer burst) {
libvirt api requires only the average property. the burst and peak are optional.
why should we demand optional fields from the user ?
Line 86: return (average != null || peak != null || burst != null)
Line 87: && (average == null || peak == null || burst == null);
Line 88: }
Line 89:
Line 94: ||
averageOutOfRange(getNetworkQoS().getOutboundAverage())
Line 95: || peakOutOfRange(getNetworkQoS().getOutboundPeak())
Line 96: || burstOutOfRange(getNetworkQoS().getOutboundBurst());
Line 97: }
Line 98:
there is a support for validating a range of input parameter based on a
configuration value.
See usage of @ConfiguredRange from SetupNetworksParameters:
@ConfiguredRange(min = 1, maxConfigValue =
ConfigValues.NetworkConnectivityCheckTimeoutInSeconds,
message = "VALIDATION.CONNECTIVITY.TIMEOUT.INVALID")
private Integer conectivityTimeout;
it will eliminate the need for negative values as well.
Line 99: private boolean averageOutOfRange(Integer average) {
Line 100: return average != null && average >
ConfigValues.MaxAverageNetworkQoSValue.getValue();
Line 101: }
Line 102:
Line 107: private boolean burstOutOfRange(Integer burst) {
Line 108: return burst != null && burst >
ConfigValues.MaxBurstNetworkQoSValue.getValue();
Line 109: }
Line 110:
Line 111: protected boolean negativeValues() {
there is already support for input parameter using the javax.validation package.
specifically the next method can be replaced with setting @Min(value = 0,
message = "...", groups = { CreateEntity.class, UpdateEntity.class })
the @Min should be declared on each of the NetworkQoS numeric members.
Line 112: return negativeValue(getNetworkQoS().getInboundAverage())
Line 113: || negativeValue(getNetworkQoS().getInboundPeak())
Line 114: || negativeValue(getNetworkQoS().getInboundBurst())
Line 115: || negativeValue(getNetworkQoS().getOutboundAverage())
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveNetworkQoSCommand.java
Line 13: }
Line 14:
Line 15: @Override
Line 16: protected boolean canDoAction() {
Line 17: if (validateNameAndStoragePoolNotNull()) {
why is it important for remove ? I'd expect to get an entity which contains
only the NetworkQos ID which is enough to identify the entity we wish to delete
in the system.
Line 18: NetworkQoS oldNetworkQoS =
getNetworkQoSDao().get(getNetworkQoS().getId());
Line 19: if (oldNetworkQoS == null) {
Line 20: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NETWORK_QOS_NOT_FOUND);
Line 21: } else if
(!oldNetworkQoS.getStoragePoolId().equals(getNetworkQoS().getStoragePoolId())) {
Line 34:
Line 35: @Override
Line 36: protected void setActionMessageParameters() {
Line 37: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__REMOVE);
Line 38: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__NETWORK_QOS);
same comment as in AddNetworkQoS command
Line 39: }
Line 40:
Line 41: @Override
Line 42: public AuditLogType getAuditLogTypeValue() {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/UpdateNetworkQoSCommand.java
Line 38:
Line 39: @Override
Line 40: protected void setActionMessageParameters() {
Line 41: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__UPDATE);
Line 42: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__NETWORK_QOS);
same comment as in AddNetworkQoS command
Line 43: }
Line 44:
Line 45: @Override
Line 46: public AuditLogType getAuditLogTypeValue() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/NetworkQoSParametersBase.java
Line 7: public class NetworkQoSParametersBase extends VdcActionParametersBase {
Line 8:
Line 9: private static final long serialVersionUID = -42449085707523901L;
Line 10: private NetworkQoS networkQoS;
Line 11: private Guid networkQoSGuid;
I don't see a need for the id in this class, by looking at the various commands.
at least not with the NetworkQoS.
Line 12:
Line 13: public NetworkQoS getNetworkQoS() {
Line 14: return networkQoS;
Line 15: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java
Line 750: WATCHDOG_EVENT(9901),
Line 751:
Line 752: //trusted service
Line 753: VDS_UNTRUSTED(10000, AuditLogTimeInterval.MINUTE.getValue()),
Line 754:
would you mind moving the network Qos before trusted service so the ordinal
number will be maintained ?
Line 755: //network Qos
Line 756: USER_ADDED_NETWORK_QOS(9920),
Line 757: USER_FAILED_TO_ADD_NETWORK_QOS(9921),
Line 758: USER_REMOVED_NETWORK_QOS(9922),
--
To view, visit http://gerrit.ovirt.org/16294
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If39d20b77d06165e4adcc27e6b6dc5458cac93d3
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-Reviewer: ofri masad <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
