Martin Peřina has posted comments on this change. Change subject: restapi: Host Power-Management Refactor (#977674) - WIP ......................................................................
Patch Set 34: (2 comments) http://gerrit.ovirt.org/#/c/27578/34/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/FenceAgent.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/FenceAgent.java: Line 16: Line 17: private static final long serialVersionUID = -5910560758520427911L; Line 18: private Guid id; Line 19: private Guid hostId; Line 20: private Integer order; > FenceAgent is a business-entity of the API layer and is generated from api. >FenceAgent is a business-entity of the API layer and is >generated from api.xsd. Entities aren't generated with any >constructors, which means they only have the default empty >constructor. I don't agree with that. FenceAgent is internal engine entity and it should not care about some external API (such as REST API). >This means that I can't enforce client code to set a value >in 'order'. If order was a primitive (int) then when the >engine receives a FenceAgent from the client with order=0, >how can the engine know if client code explicitly set this >field to 0, or simply ignored its existence, and it got the >default value? You have two options here: 1. Allow the user of REST API specify order to be able to create agents sequence as needed. In this case order should be required parameter and you should check in request validation that its value is positive integer. And I also think you should check order values sequence is OK (starting with 1 and without number gaps). For example '1,2,2' or '1,1,2' is OK but here are couple of incorrect examples: '1,3', '2,2,3' , '4,5,7') Also I assume that if you are adding/editing fence agents in webadmin, they will always have order started with 1 and without gaps. So if you would allow to add agent using REST with orders for example '1,3,3' and then user edit them in webadmin, order would be changed automatically to '1,2,2' and that would be bad. 2. Making order readonly value for REST API and handle order internally (you will need to provide methods like AddBefore/After, MoveBefore/After, ...) IMHO 1. is better solution and if we used it I don's see a problem with primitive int here (1 will be the default order value). >For this reason I prefer to leave it an Integer. >As for your second comment, firstly obviously I agree, but >I'd like to point out that for small values (I think up >to 256) a==b when 'a' and 'b' are Integers works exactly >the same as a==b when 'a' and 'b' are primitives (int) - > and we will never have more that 256 fence-agents for a host... That's true internally, but on Integer instances you should always call equals/compareTo method in you code. Otherwise you could be very surprised. Line 21: Line 22: @EditableField Line 23: private HashMap<String, String> optionsMap; Line 24: http://gerrit.ovirt.org/#/c/27578/34/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java: Line 31: /** Line 32: * <code>VdsDAODbFacadeImpl</code> provides an implementation of {@code VdsDAO} that uses previously written code from Line 33: * {@code DbFacade}. Line 34: */ Line 35: public class VdsDAODbFacadeImpl extends BaseDAODbFacade implements VdsDAO { > After having thought about it some more and consulting with Eli, I think it Well, I don't like the way 'merge first, fix later'. But if Eli and Oved are OK with that, I can live with that Line 36: Line 37: @Override Line 38: public VDS get(Guid id) { Line 39: return get(id, null, false); -- To view, visit http://gerrit.ovirt.org/27578 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id0b384347a52db8b0aa6ae684fad40a5491dd2f7 Gerrit-PatchSet: 34 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
