Alon Bar-Lev has posted comments on this change.
Change subject: engine: Add infrastructure code for removal of parameter classes
......................................................................
Patch Set 5:
(18 comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionParametersBase.java
Line 256: paramsMap.<T>put(param, value);
Line 257: return this;
Line 258: }
Line 259:
Line 260: public <T> T getParameterValue(VdcParameter<T> param) {
getParameter() ? why do we need Value for?
Line 261: return paramsMap.<T>get(param, param.getJavaType());
Line 262: }
Line 263:
Line 264: public <T> T getParameterValue(VdcParameter<T> param, Class<T>
retType) {
Line 257: return this;
Line 258: }
Line 259:
Line 260: public <T> T getParameterValue(VdcParameter<T> param) {
Line 261: return paramsMap.<T>get(param, param.getJavaType());
why can't the get call the getJavaType()?
Line 262: }
Line 263:
Line 264: public <T> T getParameterValue(VdcParameter<T> param, Class<T>
retType) {
Line 265: return paramsMap.<T>get(param, retType);
Line 264: public <T> T getParameterValue(VdcParameter<T> param, Class<T>
retType) {
Line 265: return paramsMap.<T>get(param, retType);
Line 266: }
Line 267:
Line 268: public VdcParametersMap getParamsHelper() {
getParameters() why do we need the Helper for?
Line 269: return paramsMap;
Line 270: }
Line 271:
Line 272: public void setParamsHelper(VdcParametersMap helper) {
Line 347: return false;
Line 348: } else if (!stepId.equals(other.stepId))
Line 349: return false;
Line 350: if (paramsMap == null) {
Line 351: if (other.getParamsHelper() != null)
other.paramsMap? why do you need to access via getter?
Line 352: return false;
Line 353: } else if (!paramsMap.equals(other.getParamsHelper()))
Line 354: return false;
Line 355:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/GenericVdcParameter.java
Line 3: import org.ovirt.engine.core.compat.Guid;
Line 4:
Line 5: public class GenericVdcParameter {
Line 6:
Line 7: public final static VdcParameter GUID_ID =
VdcParameter.<Guid>create("GUID_ID", Guid.class, new
Guid("00000000-0000-0000-0000-000000000001"));
why this has to be template?
Why do we need to construct GUID here and not do it within create?
Why can't it be:
public final static VdcParameter GUID_ID = VdcParameter.create("GUID_ID",
Guid.class, "00000000-0000-0000-0000-000000000001");
Line 8:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/InvalidParameterGuidException.java
Line 2:
Line 3: public class InvalidParameterGuidException extends
IllegalArgumentException {
Line 4:
Line 5: public InvalidParameterGuidException(String guid) {
Line 6: super("Invalid value for Guid of VdcParameter. A parameter with
the same Guid already exists " + guid);
this is not helpful... please print what key one trying to add and where is the
conflict. this is the reason we have names for parameters.
Line 7: }
Line 8:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/InvalidParameterRequestTypeException.java
Line 4:
Line 5: public InvalidParameterRequestTypeException(Class expected, Class<?
extends Object> actual) {
Line 6: super("Invalid return type requested for
VdcQueryParametersBase.getParameter. Expected value of type "
Line 7: + expected.getName()
Line 8: + "but requested class was "
space before but?
Line 9: + actual.getName());
Line 10: }
Line 11:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/InvalidParameterValueException.java
Line 4:
Line 5: public InvalidParameterValueException(Class expected, Class<?
extends Object> actual) {
Line 6: super("Invalid value for VdcQueryParametersBase.addParameter.
Expected value of type "
Line 7: + expected.getName()
Line 8: + "but actual class was "
space before but?
Line 9: + actual.getName());
Line 10: }
Line 11:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParameter.java
Line 4: import java.util.ArrayList;
Line 5: import java.util.List;
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7:
Line 8: public class VdcParameter<T> implements Serializable {
why does the class needs to be template?
Line 9:
Line 10: private String name;
Line 11: private Class<T> javaType;
Line 12: private Guid guid;
Line 7:
Line 8: public class VdcParameter<T> implements Serializable {
Line 9:
Line 10: private String name;
Line 11: private Class<T> javaType;
This can be simply Class no?
Line 12: private Guid guid;
Line 13: private static final List<Guid> guidsList = new ArrayList<Guid>();
Line 14:
Line 15: /**
Line 9:
Line 10: private String name;
Line 11: private Class<T> javaType;
Line 12: private Guid guid;
Line 13: private static final List<Guid> guidsList = new ArrayList<Guid>();
I suggest HashMap to contain key guid and value parameter so we can tell user
what is the conflict.
Line 14:
Line 15: /**
Line 16: * Needed by Json Serialization/Deserialization
Line 17: */
Line 17: */
Line 18: private VdcParameter() {
Line 19: }
Line 20:
Line 21: private VdcParameter(String name, Class<T> javaType, Guid guid) {
Class should be enough.
Line 22: this.name = name;
Line 23: this.javaType = javaType;
Line 24: this.guid = guid;
Line 25: }
Line 49: }
Line 50:
Line 51: @Override
Line 52: public String toString() {
Line 53: return getClass().getName() + "." + getName();
please also add the GUID which is important.
Line 54: }
Line 55:
Line 56: public static <T> VdcParameter<T> create(String name, Class<T>
javaType, Guid guid) {
Line 57: if (guidsList.contains(guid)) {
Line 52: public String toString() {
Line 53: return getClass().getName() + "." + getName();
Line 54: }
Line 55:
Line 56: public static <T> VdcParameter<T> create(String name, Class<T>
javaType, Guid guid) {
This should be simple to use...
public static VdcParameter create(String name, Class type, String guid)
Line 57: if (guidsList.contains(guid)) {
Line 58: throw new InvalidParameterGuidException(guid.toString());
Line 59: }
Line 60: guidsList.add(guid);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParametersMap.java
Line 9: import org.ovirt.engine.core.compat.Guid;
Line 10:
Line 11: public class VdcParametersMap implements Serializable {
Line 12:
Line 13: private Map<Guid, VdcParameterValue> paramsMap;
I still do not understand why can't we have VdcParameter as a key and Object as
a value.
The equals method and the hash of VdcParameter can be based on the Guid.
Line 14:
Line 15: public VdcParametersMap() {
Line 16: paramsMap = new HashMap<Guid, VdcParameterValue>();
Line 17: }
Line 15: public VdcParametersMap() {
Line 16: paramsMap = new HashMap<Guid, VdcParameterValue>();
Line 17: }
Line 18:
Line 19: public <T> void put(VdcParameter param, T value) {
I do not understand why is this template, I expect:
public void put(VdcParameter param, Object value)
Line 20: VdcParameterValue.raiseExceptionIfInvalidObjectType(param,
value);
Line 21: paramsMap.put(param.getGuid(), new VdcParameterValue<T>(param,
value));
Line 22: }
Line 23:
Line 20: VdcParameterValue.raiseExceptionIfInvalidObjectType(param,
value);
Line 21: paramsMap.put(param.getGuid(), new VdcParameterValue<T>(param,
value));
Line 22: }
Line 23:
Line 24: public <T> T get(VdcParameter<T> param) {
Why can't it be something like:
public <T> T get(VdcParameter param) {
VdcParameterValue.raiseExceptionIfInvalidReturnType(
param.getJavaType(),
T
);
return (T)paramsMap.get(param.getGuid()).getValue();
}
and drop function bellow.
Line 25: return get(param, param.getJavaType());
Line 26: }
Line 27:
Line 28: /**
Line 37: }
Line 38: return paramValue.getValue();
Line 39: }
Line 40:
Line 41: public int getNumParameterValues() {
size() ?
Line 42: return paramsMap.size();
Line 43: }
Line 44:
Line 45: @Override
--
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: 5
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: Liran Zelkha <liran.zel...@gmail.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@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
