Michael Pasternak has posted comments on this change.
Change subject: restapi: Support for gluster hooks added
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(21 inline comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 3153:
Line 3154: <xs:element name="hook_status" type="HookStatus"/>
Line 3155: <xs:complexType name="HookStatus">
Line 3156: <xs:sequence>
Line 3157: <xs:element name="hook_status" type="xs:string" minOccurs="0"
maxOccurs="unbounded">
please reuse <status> element instead of using your own
Line 3158: <xs:annotation>
Line 3159: <xs:appinfo>
Line 3160: <jaxb:property name="GlusterHookStatus"/>
Line 3161: </xs:appinfo>
Line 3203: <xs:complexContent>
Line 3204: <xs:extension base="BaseResource">
Line 3205: <xs:sequence>
Line 3206: <xs:element ref="host" minOccurs="0" maxOccurs="1"/>
Line 3207: <xs:element ref="content_types" minOccurs="0"
maxOccurs="1" />
shouldn't it be "content_type"?
Line 3208: <xs:element ref="hook_status" minOccurs="0" maxOccurs="1"/>
Line 3209: <xs:element name="checksum" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 3210: </xs:sequence>
Line 3211: </xs:extension>
Line 3204: <xs:extension base="BaseResource">
Line 3205: <xs:sequence>
Line 3206: <xs:element ref="host" minOccurs="0" maxOccurs="1"/>
Line 3207: <xs:element ref="content_types" minOccurs="0"
maxOccurs="1" />
Line 3208: <xs:element ref="hook_status" minOccurs="0" maxOccurs="1"/>
please reuse <status> element instead of using your own
Line 3209: <xs:element name="checksum" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 3210: </xs:sequence>
Line 3211: </xs:extension>
Line 3212: </xs:complexContent>
Line 3223: <xs:extension base="BaseResource">
Line 3224: <xs:sequence>
Line 3225: <xs:element ref="cluster" minOccurs="0" maxOccurs="1"/>
Line 3226: <xs:element name="gluster_command" type="xs:string"
minOccurs="0" maxOccurs="1"/>
Line 3227: <xs:element name="gluster_stage" type="xs:string"
minOccurs="0" maxOccurs="1"/>
why you always prefix element names with 'gluster_'?, i can't see any
justification for this, please remove it from all types.
(to be honest i'm also not sure naming new types Gluster_ is a right thing to
do, reusing it later by other APIs is misleading)
Line 3228: <xs:element name="content_type" type="xs:string"
minOccurs="0" maxOccurs="1"/>
Line 3229: <xs:element name="checksum" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 3230: <xs:element name="content" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 3231: <xs:element name="conflict_status" type="xs:unsignedShort"
minOccurs="0" maxOccurs="1"/>
Line 3229: <xs:element name="checksum" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 3230: <xs:element name="content" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 3231: <xs:element name="conflict_status" type="xs:unsignedShort"
minOccurs="0" maxOccurs="1"/>
Line 3232: <xs:element name="conflicts" type="xs:string"
minOccurs="0" maxOccurs="1"/>
Line 3233: <xs:element name="hook_status" type="xs:string"
minOccurs="0" maxOccurs="1"/>
please reuse <status> element instead of using your own
Line 3234: <xs:element ref="server_hooks" minOccurs="0"
maxOccurs="1"/>
Line 3235: </xs:sequence>
Line 3236: </xs:extension>
Line 3237: </xs:complexContent>
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/gluster/BackendGlusterHookResource.java
Line 43:
Line 44: @Override
Line 45: public GlusterHook get() {
Line 46: GlusterHook hook = performGet(VdcQueryType.GetGlusterHookById,
new GlusterHookQueryParameters(guid,true));
Line 47: VdcQueryReturnValue result =
runQuery(VdcQueryType.GetGlusterHookContent, new
GlusterHookContentQueryParameters(guid));
why this is done in two separate quires?
Line 48: if (result != null
Line 49: && result.getSucceeded()
Line 50: && result.getReturnValue() != null) {
Line 51: hook.setContent((String)result.getReturnValue());
Line 66: @Override
Line 67: public Response resolve(Action action) {
Line 68: validateParameters(action, "resolutionType");
Line 69:
Line 70: ResolutionType resolutionType =
validateEnum(ResolutionType.class, action.getResolutionType().toUpperCase());
you could use a second signature where passing boolean true would validate enum
transferring it to the UpperCase (so you, won't have to do it manually)
Line 71:
Line 72: switch (resolutionType) {
Line 73: case ADD:
Line 74: return addToMissingServers(action);
Line 84: }
Line 85:
Line 86: private Response copy(Action action) {
Line 87: GlusterHookManageParameters params = new
GlusterHookManageParameters(guid);
Line 88: if (action.getHost() != null) {
action.isSetHost()
Line 89: validateParameters(action.getHost(),"host.id|name");
Line 90: Guid hostId = getHostId(action);
Line 91: if (hostId == null) {
Line 92: return handleError(new
EntityNotFoundException(action.getHost().getId() != null ? id :
action.getHost().getName()), false);
Line 87: GlusterHookManageParameters params = new
GlusterHookManageParameters(guid);
Line 88: if (action.getHost() != null) {
Line 89: validateParameters(action.getHost(),"host.id|name");
Line 90: Guid hostId = getHostId(action);
Line 91: if (hostId == null) {
no need for this validation, cause getHostId(Action action) returns new UUID
from string in action.host.id or fetches host.id by name from action.host.name
and if such host doesn't exist throws 404
Line 92: return handleError(new
EntityNotFoundException(action.getHost().getId() != null ? id :
action.getHost().getName()), false);
Line 93: }
Line 94: params.setSourceServerId(hostId);
Line 95: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/gluster/BackendGlusterHooksResource.java
Line 38:
Line 39: public BackendGlusterHooksResource(ClusterResource parent, String
clusterId) {
Line 40: this();
Line 41: setParent(parent);
Line 42: this.clusterId = clusterId;
no need to save clusterId here as parent already has it.
Line 43: }
Line 44:
Line 45: public ClusterResource getParent() {
Line 46: return parent;
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/gluster/BackendGlusterHookResourceTest.java
Line 72: verifyActionResponse(resource.resolve(action));
Line 73: }
Line 74:
Line 75: @Test
Line 76: public void testAdd() throws Exception {
i'd call it testResolveAdd() otherwise it''s misleading, also what about other
resolution tests?
Line 77:
setUriInfo(setUpActionExpectations(VdcActionType.AddGlusterHook,
Line 78: GlusterHookManageParameters.class,
Line 79: new String[] { "HookId" },
Line 80: new Object[] { hookId}));
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GlusterHookMapper.java
Line 15: public static GlusterHookEntity map(GlusterHook hook,
GlusterHookEntity entity) {
Line 16: GlusterHookEntity hookEntity = entity != null ? entity : new
GlusterHookEntity();
Line 17:
Line 18: if (hook.isSetId()) {
Line 19:
hookEntity.setId(Guid.createGuidFromStringDefaultEmpty(hook.getId()));
please use GuidUtils to populate UUID from string
Line 20: }
Line 21:
Line 22: if (hook.isSetName()) {
Line 23: hookEntity.setName(hook.getName());
Line 60: }
Line 61:
Line 62: if (entity.getStage() != null) {
Line 63: model.setGlusterStage(map(entity.getStage(),null));
Line 64: }
any reason for not mapping it on opposite direction?
Line 65:
Line 66: if (entity.getStatus() != null) {
Line 67: model.setHookStatus(map(entity.getStatus(),null));
Line 68: }
Line 68: }
Line 69:
Line 70: if (entity.getContentType() != null) {
Line 71: model.setContentType(map(entity.getContentType(), null));
Line 72: }
any reason for not mapping it on opposite direction?
Line 73:
Line 74: if (entity.getChecksum() != null) {
Line 75: model.setChecksum(entity.getChecksum());
Line 76: }
Line 118: return serverHookModel;
Line 119: }
Line 120:
Line 121: @Mapping(from =
org.ovirt.engine.core.common.businessentities.gluster.GlusterHookContentType.class,
to = String.class)
Line 122: public static String
map(org.ovirt.engine.core.common.businessentities.gluster.GlusterHookContentType
hookContentType,
this mapping should happen via public enum and not via mapping backend enum to
string
Line 123: String template) {
Line 124: switch (hookContentType) {
Line 125: case BINARY:
Line 126: return
org.ovirt.engine.core.common.businessentities.gluster.GlusterHookContentType.BINARY.toString();
Line 131: }
Line 132: }
Line 133:
Line 134: @Mapping(from =
org.ovirt.engine.core.common.businessentities.gluster.GlusterHookStage.class,
to = String.class)
Line 135: public static String
map(org.ovirt.engine.core.common.businessentities.gluster.GlusterHookStage
hookStage,
this mapping should happen via public enum and not via mapping backend enum to
string
Line 136: String template) {
Line 137: switch (hookStage) {
Line 138: case POST:
Line 139: return
org.ovirt.engine.core.common.businessentities.gluster.GlusterHookStage.POST.toString();
Line 144: }
Line 145: }
Line 146:
Line 147: @Mapping(from =
org.ovirt.engine.core.common.businessentities.gluster.GlusterHookStatus.class,
to = String.class)
Line 148: public static String
map(org.ovirt.engine.core.common.businessentities.gluster.GlusterHookStatus
hookStatus,
this mapping should happen via public enum and not via mapping backend enum to
string
Line 149: String template) {
Line 150: switch (hookStatus) {
Line 151: case DISABLED:
Line 152: return
org.ovirt.engine.core.common.businessentities.gluster.GlusterHookStatus.DISABLED.toString();
....................................................
Commit Message
Line 14:
Line 15: This patch adds API support for managing gluster hooks.
Line 16: - api/cluster/{id}/glusterhooks
Line 17: lists all gluster hooks in the cluster along with
Line 18: conflict status
how do you add the hook/s which are yet not exist? (to all clusters)
Line 19: - api/cluster/{id}/glusterhooks/{glusterhook:id}|rel=delete
Line 20: resolve missing hook conflict by removing hook script from
Line 21: all servers and engine database
Line 22: - api/cluster/{id}/glusterhooks/{id}
Line 17: lists all gluster hooks in the cluster along with
Line 18: conflict status
Line 19: - api/cluster/{id}/glusterhooks/{glusterhook:id}|rel=delete
Line 20: resolve missing hook conflict by removing hook script from
Line 21: all servers and engine database
shouldn't this one should go to resolve() action with resolution type =
'delete'?
according to description this is just another conflict resolution also ADD is
done by the resolve() action as well.
Line 22: - api/cluster/{id}/glusterhooks/{id}
Line 23: show details of a gluster hook
Line 24:
Line 25: actions supported:
Line 27: enables hook in all servers in the cluster
Line 28: - disable
Line 29: disables hook in all servers in the cluster
Line 30: - resolve
Line 31: action.resolutionType = COPY
please remove TAB(s)
Line 32: resolve content conflict by copying content from either
Line 33: engine or from one of the servers to all or servers where
Line 34: the hook is missing as applicable (based on parameters)
Line 35: action.resolutionType = ADD
Line 34: the hook is missing as applicable (based on parameters)
Line 35: action.resolutionType = ADD
Line 36: resolve missing hook conflict by copying hook stored in
Line 37: engine database to all servers where the hook is missing
Line 38:
please also:
1. add new feature at FeatureHelper
2. describe new ENUMs at the /capabilities resource
Line 39: Change-Id: Ibf9c0299977448e0b50feeed6e38a015c60b86fd
--
To view, visit http://gerrit.ovirt.org/16512
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf9c0299977448e0b50feeed6e38a015c60b86fd
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sahina Bose <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Sahina Bose <[email protected]>
Gerrit-Reviewer: Shubhendu Tripathi <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
