Gilad Chaplik has posted comments on this change. Change subject: common: introduce qos package, and storage qos ......................................................................
Patch Set 6: (10 comments) http://gerrit.ovirt.org/#/c/27093/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/qos/QosBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/qos/QosBase.java: Line 23: @NotNull(message = "ACTION_TYPE_FAILED_QOS_INVALID_DC_ID") Line 24: private Guid storagePoolId; Line 25: Line 26: @NotNull(message = "QOS_NAME_NOT_NULL") Line 27: @Size(min = 1, max = BusinessEntitiesDefinitions.GENERAL_NAME_SIZE, message = "QOS_NAME_TOO_LONG") > what if the size is 0? (wrong message) first of all this is a right message (name.size = 0 -> name is invalid (!valid)), and I think it gets covered in the other validations anyway. Line 28: @ValidI18NName(message = "QOS_NAME_INVALID") Line 29: private String name; Line 30: Line 31: @Size(max = BusinessEntitiesDefinitions.GENERAL_MAX_SIZE) Line 65: Line 66: /** Line 67: * @return derived values hash code Line 68: */ Line 69: protected abstract int valuesHashCode(); > Shouldn't this method be used in hashCode method? For example: will use the conventional approach Done. Line 70: Line 71: @Override Line 72: public Guid getId() { Line 73: return id; Line 107: return getId(); Line 108: } Line 109: Line 110: @Override Line 111: public int hashCode() { > atleast in theory, we might have same hashcode for unequal qos objects whic Done Line 112: final int prime = 31; Line 113: int result = 1; Line 114: result = prime * result + ((description == null) ? 0 : description.hashCode()); Line 115: result = prime * result + ((id == null) ? 0 : id.hashCode()); Line 118: result = prime * result + ((storagePoolId == null) ? 0 : storagePoolId.hashCode()); Line 119: return result; Line 120: } Line 121: Line 122: @Override > to make it short - i'm also in for "traditional" implementation for both eq Done Line 123: public boolean equals(Object obj) { Line 124: if (this == obj) Line 125: return true; Line 126: if (obj == null) http://gerrit.ovirt.org/#/c/27093/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/qos/QosType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/qos/QosType.java: Line 24: public int getValue() { Line 25: return value; Line 26: } Line 27: Line 28: public static QosType forValue(int value) { > when you rebase you have this as an Integer, as autoboxing will happen anyw what is your recommended behavior? Line 29: return valueToStatus.get(value); Line 30: } http://gerrit.ovirt.org/#/c/27093/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/qos/StorageQos.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/qos/StorageQos.java: Line 12: super(QosType.STORAGE); Line 13: } Line 14: Line 15: private static final long serialVersionUID = 1122123549710787758L; Line 16: > general: please set the relevant validation groups for the validators (Crea what is the default? if it's both, you've got your answer. Line 17: @ConfiguredRange(min = 0, maxConfigValue = ConfigValues.maxThroughputUpperBoundQosValue, Line 18: message = "ACTION_TYPE_FAILED_QOS_OUT_OF_RANGE_VALUES") Line 19: private Integer maxThroughput; Line 20: Line 100: && ObjectUtils.objectsEqual(maxWriteIops, other.getMaxWriteIops()); Line 101: } Line 102: Line 103: @Override Line 104: public int valuesHashCode() { > doesn't seems like it's somewhere used. will follow traditional way. Line 105: final int prime = 31; Line 106: int result = 1; Line 107: result = prime * result + ((maxThroughput == null) ? 0 : maxThroughput.hashCode()); Line 108: result = prime * result + ((maxReadThroughput == null) ? 0 : maxReadThroughput.hashCode()); http://gerrit.ovirt.org/#/c/27093/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java: Line 1750: @TypeConverterAttribute(Integer.class) Line 1751: @DefaultValueAttribute("1500") Line 1752: DefaultMtu, Line 1753: Line 1754: @DefaultValueAttribute("1000000") > missing @TypeConverterAttribute(Integer.class) Done, 10x! Line 1755: maxThroughputUpperBoundQosValue, Line 1756: Line 1757: @TypeConverterAttribute(Integer.class) Line 1758: @DefaultValueAttribute("1000000") http://gerrit.ovirt.org/#/c/27093/6/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 1094: ACTION_TYPE_FAILED_GLUSTER_VOLUME_CANNOT_STOP_REBALANCE_IN_PROGRESS= Cannot ${action} ${type}. Rebalance is running on the volume ${volumeName} in cluster ${vdsGroup}. Line 1095: ACTION_TYPE_FAILED_GLUSTER_OPERATION_INPROGRESS=Cannot ${action} ${type}. Gluster operation is in progress in cluster. Please try again. Line 1096: ACTION_TYPE_FAILED_TAG_ID_REQUIRED=Cannot ${action} ${type}. Tag ID is required. Line 1097: Line 1098: ACTION_TYPE_FAILED_QOS_OUT_OF_RANGE_VALUES=Cannot ${action} ${type}. Values are out of range. > I'd add specific message for each one, it may not be informative enough.. Moti already commented about it. I will open a 3.5 targeted bug. Line 1099: ACTION_TYPE_FAILED_QOS_INVALID_DC_ID=Cannot ${action} ${type}. Invalid data center. Line 1100: Line 1101: ACTION_TYPE_FAILED_NETWORK_QOS_MISSING_VALUES=Cannot ${action} ${type}. All three values are needed in order to define QoS on each network directions. Line 1102: ACTION_TYPE_FAILED_NETWORK_QOS_NEGATIVE_VALUES=Cannot ${action} ${type}. Negative values are not allowed. Line 1095: ACTION_TYPE_FAILED_GLUSTER_OPERATION_INPROGRESS=Cannot ${action} ${type}. Gluster operation is in progress in cluster. Please try again. Line 1096: ACTION_TYPE_FAILED_TAG_ID_REQUIRED=Cannot ${action} ${type}. Tag ID is required. Line 1097: Line 1098: ACTION_TYPE_FAILED_QOS_OUT_OF_RANGE_VALUES=Cannot ${action} ${type}. Values are out of range. Line 1099: ACTION_TYPE_FAILED_QOS_INVALID_DC_ID=Cannot ${action} ${type}. Invalid data center. > you can use ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST message prefer to have a separate message (using my role as a UI maintainer for a second) Line 1100: Line 1101: ACTION_TYPE_FAILED_NETWORK_QOS_MISSING_VALUES=Cannot ${action} ${type}. All three values are needed in order to define QoS on each network directions. Line 1102: ACTION_TYPE_FAILED_NETWORK_QOS_NEGATIVE_VALUES=Cannot ${action} ${type}. Negative values are not allowed. Line 1103: ACTION_TYPE_FAILED_NETWORK_QOS_OUT_OF_RANGE_VALUES=Cannot ${action} ${type}. Values are out of range. -- To view, visit http://gerrit.ovirt.org/27093 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a9af59277b5055453159f002f19046c0051d8ff Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: [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
