Alon Bar-Lev has posted comments on this change.
Change subject: engine: Add infrastructure code for removal of parameter classes
......................................................................
Patch Set 1:
(15 comments)
....................................................
Commit Message
Line 5: CommitDate: 2013-10-22 19:44:52 -0400
Line 6:
Line 7: engine: Add infrastructure code for removal of parameter classes
Line 8:
Line 9: This patch adds the infrastructure code that is
you don't need to specify 'this patch'... of course it is a patch... :)
Line 10: needed by subsequent patches to remove
Line 11: dependency on parameter classes.
Line 12:
Line 13: A new map Map<VdcQueryParameter, VdcQueryParameterValue>
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java
Line 268:
Line 269: public VdcParametersMapBase getParamsHelper() {
Line 270: return paramsMap;
Line 271: }
Line 272:
question... why can't we have:
xxx.getParameters().getParameterValue()
why do we need these bridge functions?
isn't there in java7 something like xxx.parameters.getParameterValue() so
parameters is actually getter?
Line 273: @Override
Line 274: public int hashCode() {
Line 275: final int prime = 31;
Line 276: int result = 1;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/InvalidParameterValueException.java
Line 3: public class InvalidParameterValueException extends
IllegalArgumentException {
Line 4:
Line 5: static final String MSG_PREFIX = "Invalid value for
VdcQueryParametersBase.addParameter. Expected value of type ";
Line 6: public InvalidParameterValueException(Class expected, Class<?
extends Object> actual) {
Line 7: super(MSG_PREFIX+expected.getName()+" but actual class was
"+actual.getName());
I prefer using String.format, it is true that I come from C, but formatted
messages are way more readable than concatenations.
Line 8: }
Line 9:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParameter.java
Line 7: public enum VdcParameter {
Line 8: ClusterId(Guid.class, new VdcParameterHandler<Guid>()),
Line 9: DataCenterId(Guid.class, new VdcParameterHandler<Guid>()),
Line 10: RoleId(Guid.class, new VdcParameterHandler<Guid>()),
Line 11: StorageDomainId(Guid.class, new VdcParameterHandler<Guid>());
why are these here?
Line 12:
Line 13: public static enum Status { Active, Obsolete}
Line 14:
Line 15: private Class javaType;
Line 9: DataCenterId(Guid.class, new VdcParameterHandler<Guid>()),
Line 10: RoleId(Guid.class, new VdcParameterHandler<Guid>()),
Line 11: StorageDomainId(Guid.class, new VdcParameterHandler<Guid>());
Line 12:
Line 13: public static enum Status { Active, Obsolete}
can we use the @Deprecated of java?
Line 14:
Line 15: private Class javaType;
Line 16: private Status status;
Line 17: private VdcParameterHandler parameterHandler;
Line 12:
Line 13: public static enum Status { Active, Obsolete}
Line 14:
Line 15: private Class javaType;
Line 16: private Status status;
we need guid as well for serialziation.
Line 17: private VdcParameterHandler parameterHandler;
Line 18:
Line 19: <T> VdcParameter(Class javaType, VdcParameterHandler<T>
parameterHandler) {
Line 20: this(javaType, Status.Active, parameterHandler);
Line 13: public static enum Status { Active, Obsolete}
Line 14:
Line 15: private Class javaType;
Line 16: private Status status;
Line 17: private VdcParameterHandler parameterHandler;
can we use annotations or is it more complex?
Line 18:
Line 19: <T> VdcParameter(Class javaType, VdcParameterHandler<T>
parameterHandler) {
Line 20: this(javaType, Status.Active, parameterHandler);
Line 21: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParameterValue.java
Line 27: public int hashCode() {
Line 28: int hash = 7;
Line 29: hash = 31 * hash + (this.value != null ? this.value.hashCode()
: 0);
Line 30: return hash;
Line 31: }
why not just return Object? Why do we need this class?
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParametersMapBase.java
Line 5: import java.util.Map;
Line 6: import java.util.Map.Entry;
Line 7: import
org.ovirt.engine.core.common.queries.InvalidParameterValueException;
Line 8:
Line 9: public class VdcParametersMapBase implements Serializable {
Why MapBase? Why not just VdcParameters?
Line 10:
Line 11: private Map<VdcParameter, VdcParameterValue> paramsMap;
Line 12:
Line 13: public VdcParametersMapBase() {
Line 14: paramsMap = new EnumMap<VdcParameter,
VdcParameterValue>(VdcParameter.class);
Line 15: }
Line 16:
Line 17: public <T> void addParameter(VdcParameter param,
VdcParameterValue<T> value) {
Line 18: if (!value.getValue().getClass().equals(param.getJavaType()))
I think someone worked hard to put {} at all nested statements...
Line 19: throw new
InvalidParameterValueException(param.getJavaType(),
value.getValue().getClass());
Line 20: paramsMap.put(param, value);
Line 21: }
Line 22:
Line 20: paramsMap.put(param, value);
Line 21: }
Line 22:
Line 23: public <T> VdcParameterValue<T> getParameterValue(VdcParameter
param) {
Line 24: return paramsMap.get(param);
I would like get to raise an exception if T is wrong
Line 25: }
Line 26:
Line 27: public int getNumParameterValues() {
Line 28: return paramsMap.size();
Line 39: VdcParametersMapBase other = (VdcParametersMapBase) obj;
Line 40: if (getNumParameterValues() != other.getNumParameterValues()) {
Line 41: return false;
Line 42: }
Line 43: for (Entry<VdcParameter, VdcParameterValue> entry :
paramsMap.entrySet()) {
why can't you use the equals of map?
Line 44: VdcParameterValue otherValue =
other.getParameterValue(entry.getKey());
Line 45: if (otherValue == null) {
Line 46: return false;
Line 47: }
Line 57: int hash = 7;
Line 58: final int prime = 31;
Line 59: for (Entry<VdcParameter, VdcParameterValue> entry :
paramsMap.entrySet()) {
Line 60: hash = prime * hash + entry.getValue().hashCode();
Line 61: }
why can't we the hasCode of map?
Line 62: return hash;
Line 63: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/AbstractBackendSubResource.java
Line 18: public abstract class AbstractBackendSubResource<R extends
BaseResource, Q /* extends IVdcQueryable */> extends
Line 19: AbstractBackendResource<R, Q> {
Line 20:
Line 21: protected static final String[] STRICTLY_IMMUTABLE = { "id" };
Line 22: protected static final Log LOG =
LogFactory.getLog(AbstractBackendSubResource.class);
I am unsure this belongs to this patch
Line 23:
Line 24: protected String id;
Line 25: protected Guid guid;
Line 26:
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml
Line 202
Line 203
Line 204
Line 205
Line 206
are we actually use the parameter classes in gwt?!?!
--
To view, visit http://gerrit.ovirt.org/20414
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a0970e492c0eff561888a46b02e47645ff68fc3
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
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
