Ravi Nori has posted comments on this change.

Change subject: engine: Add infrastructure code for removal of parameter classes
......................................................................


Patch Set 12:

(8 comments)

....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/InvalidParameterGuidException.java
Line 1: package org.ovirt.engine.core.common.utils;
Line 2: 
Line 3: public class InvalidParameterGuidException extends 
IllegalArgumentException {
Line 4: 
Line 5:     public InvalidParameterGuidException(String name, VdcParameter 
param) {
This is a RTE that is not supposed to happen. I dont think the user should 
catch it and handle it. So I didn't bother with saving the parameters. Do you 
want me to hold them in class level variables and provide getters for them?
Line 6:         super("Invalid value for Guid "+param.getGuid()+" for Parameter 
" + name +
Line 7:                 ". A parameter with the same Guid already exists " + 
param.getName());
Line 8:     }
Line 9: 


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParameter.java
Line 49:     }
Line 50: 
Line 51:     @Override
Line 52:     public String toString() {
Line 53:         return serialize(this);
Will change
Line 54:     }
Line 55: 
Line 56:     public static VdcParameter create(String name, Class javaType, 
String guidStr) {
Line 57:         Guid guid = new Guid(guidStr);


Line 55: 
Line 56:     public static VdcParameter create(String name, Class javaType, 
String guidStr) {
Line 57:         Guid guid = new Guid(guidStr);
Line 58:         synchronized (parametersMap) {
Line 59:             if (parametersMap.containsKey(guid)) {
You want this?



VdcParameter param = parametersMap.get(guid);
if (param != null) {
    throw new InvalidParameterGuidException(name, parametersMap.get(guid));
}
param = new VdcParameter(name, javaType, guid);
parametersMap.put(guid, param);
return param;
Line 60:                 throw new InvalidParameterGuidException(name, 
parametersMap.get(guid));
Line 61:             }
Line 62:             VdcParameter param = new VdcParameter(name, javaType, 
guid);
Line 63:             parametersMap.put(guid, param);


Line 83:         }
Line 84: 
Line 85:         switch (Integer.parseInt(map.get(VERSION_KEY))) {
Line 86:             case 0:
Line 87:                 synchronized (parametersMap) {
As far as I know GWT does not have concurrentHashMap.
Line 88:                     Guid guid = new Guid(map.get(GUID_KEY));
Line 89:                     if (!parametersMap.containsKey(guid)) {
Line 90:                         throw new 
InvalidParameterKeyException("VdcParameter with guid not found "+ 
guid.toString());
Line 91:                     }


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/VdcParametersMap.java
Line 40:             throw new 
InvalidParameterValueException(param.getJavaType(), value.getClass());
Line 41:         }
Line 42:     }
Line 43: 
Line 44:     private void raiseExceptionIfInvalidReturnType(Class javaType, 
Class valueType) {
ok
Line 45:         if (!valueType.equals(javaType)) {
Line 46:             throw new InvalidParameterRequestTypeException(javaType, 
valueType);
Line 47:         }
Line 48:     }


Line 62:     public boolean equals(Object obj) {
Line 63:         if (this == obj) {
Line 64:             return true;
Line 65:         }
Line 66:         if (obj == null || getClass() != obj.getClass()) {
Almost everywhere in our code base and in JDK's equals methods we have  
getClass() !=  obj.getClass()

So I am just following conventions
Line 67:             return false;
Line 68:         }
Line 69:         return paramsMap.equals(((VdcParametersMap) obj).paramsMap);
Line 70:     }


....................................................
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/serialization/json/JsonObjectDeserializer.java
Line 53:         
formattedMapper.getDeserializationConfig().addMixInAnnotations(VmStatic.class, 
JsonVmStaticMixIn.class);
Line 54:         formattedMapper.configure(Feature.FAIL_ON_UNKNOWN_PROPERTIES, 
false);
Line 55:         SimpleModule module = new SimpleModule("VdcParameterModule", 
new Version(1, 0, 0, "M"));
Line 56:         module.addKeyDeserializer(VdcParameter.class, new 
VdcParameterDeserializer());
Line 57:         formattedMapper.registerModule(module);
I dont see any annotation for custom KeyDeserializer in Jackson
Line 58:         formattedMapper.enableDefaultTyping();
Line 59:     }
Line 60: 
Line 61:     @Override


....................................................
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/serialization/json/JsonObjectSerializer.java
Line 99:     public String serializeUnformattedJson(Serializable payload) 
throws SerializationExeption {
Line 100:         return writeJsonAsString(payload, unformattedMapper);
Line 101:     }
Line 102: 
Line 103:     private static class VdcParameterSerializer extends 
JsonSerializer<VdcParameter> {
I dont see any annotation to replace custom serialization in Jackson.

This is very specific to JsonSerialization do you really want to put this in 
VdcParameters?
Line 104:         @Override
Line 105:         public void serialize(VdcParameter key, JsonGenerator jg, 
SerializerProvider sp) throws IOException {
Line 106:             jg.writeFieldName(VdcParameter.serialize(key));
Line 107:         }


-- 
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: 12
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 <lzel...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: mooli tayer <mta...@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

Reply via email to