Michael Pasternak has posted comments on this change.
Change subject: API: Adding support for external events
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(11 inline comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 1458: <xs:element ref="template" minOccurs="0" />
Line 1459: <xs:element ref="cluster" minOccurs="0" />
Line 1460: <xs:element ref="data_center" minOccurs="0" />
Line 1461: <xs:element name="origin" type="xs:string" minOccurs="0"
maxOccurs="1" />
Line 1462: <xs:element name="custom_event_id" type="xs:int"
minOccurs="0" maxOccurs="1" />
i'd go with 'custom_id'
Line 1463: <xs:element name="event_flood_in_sec" type="xs:int"
minOccurs="0" maxOccurs="1" />
Line 1464: <xs:element name="custom_data" type="xs:string"
minOccurs="0" maxOccurs="1" />
Line 1465: </xs:sequence>
Line 1466: </xs:extension>
Line 1459: <xs:element ref="cluster" minOccurs="0" />
Line 1460: <xs:element ref="data_center" minOccurs="0" />
Line 1461: <xs:element name="origin" type="xs:string" minOccurs="0"
maxOccurs="1" />
Line 1462: <xs:element name="custom_event_id" type="xs:int"
minOccurs="0" maxOccurs="1" />
Line 1463: <xs:element name="event_flood_in_sec" type="xs:int"
minOccurs="0" maxOccurs="1" />
i'd go 'flood' or 'flood_rate' or similar (we try to have only two period based
element names in api, unless it's exceptional case)
Line 1464: <xs:element name="custom_data" type="xs:string"
minOccurs="0" maxOccurs="1" />
Line 1465: </xs:sequence>
Line 1466: </xs:extension>
Line 1467: </xs:complexContent>
Line 1460: <xs:element ref="data_center" minOccurs="0" />
Line 1461: <xs:element name="origin" type="xs:string" minOccurs="0"
maxOccurs="1" />
Line 1462: <xs:element name="custom_event_id" type="xs:int"
minOccurs="0" maxOccurs="1" />
Line 1463: <xs:element name="event_flood_in_sec" type="xs:int"
minOccurs="0" maxOccurs="1" />
Line 1464: <xs:element name="custom_data" type="xs:string"
minOccurs="0" maxOccurs="1" />
whitespace
Line 1465: </xs:sequence>
Line 1466: </xs:extension>
Line 1467: </xs:complexContent>
Line 1468: </xs:complexType>
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata_v-3.1.yaml
Line 1252: parameterType: null
Line 1253: signatures: []
Line 1254: urlparams: {}
Line 1255: headers: {}
Line 1256:
please remove this ^ new-line
Line 1257: - name: /api/events|rel=add
Line 1258: request:
Line 1259: body:
Line 1260: parameterType: Event
Line 1258: request:
Line 1259: body:
Line 1260: parameterType: Event
Line 1261: signatures:
Line 1262: - mandatoryArguments: {event.description: 'xs:string',
event.severity: 'xs:string', event.origin: 'xs:string', event.customEventId:
'xs:int'}
please see my comment on mandatory fields in add() method
Line 1263: optionalArguments:{event.host.id: 'xs:string',event.user.id:
'xs:string',event.vm.id: 'xs:string',event.storageDomain.id:
'xs:string',event.template.id: 'xs:string',event.cluster.id:
'xs:string',event.dataCenter.id: 'xs:string',}
Line 1264: urlparams: {}
Line 1265: headers:
Line 1266: Content-Type: {value: application/xml|json, required: true}
Line 1259: body:
Line 1260: parameterType: Event
Line 1261: signatures:
Line 1262: - mandatoryArguments: {event.description: 'xs:string',
event.severity: 'xs:string', event.origin: 'xs:string', event.customEventId:
'xs:int'}
Line 1263: optionalArguments:{event.host.id: 'xs:string',event.user.id:
'xs:string',event.vm.id: 'xs:string',event.storageDomain.id:
'xs:string',event.template.id: 'xs:string',event.cluster.id:
'xs:string',event.dataCenter.id: 'xs:string',}
are you using all these optional fields in your add command in the backend?
Line 1264: urlparams: {}
Line 1265: headers:
Line 1266: Content-Type: {value: application/xml|json, required: true}
Line 1267: Expect: {value: 201-created, required: false}
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendEventsResource.java
Line 60: }
Line 61:
Line 62: @Override
Line 63: public Response add(Event event) {
Line 64: validateParameters(event, "origin", "severity",
"eventFloodInSec", "description");
- no need to force user pass "origin" (unless i'm missing something), we can
define it here as only custom events are being added.
- really? 'description' is mandatory?
Line 65: AuditLog entity = map(event);
Line 66: return performCreation(VdcActionType.AddExternalEvent,
Line 67: new AddExternalEventParameters(entity),
Line 68: new
QueryIdResolver<Long>(VdcQueryType.GetAuditLogById,
GetAuditLogByIdParameters.class));
Line 61:
Line 62: @Override
Line 63: public Response add(Event event) {
Line 64: validateParameters(event, "origin", "severity",
"eventFloodInSec", "description");
Line 65: AuditLog entity = map(event);
i'd prefer you not creating variable here, but pass map(event) as param to
parameters ctr.
Line 66: return performCreation(VdcActionType.AddExternalEvent,
Line 67: new AddExternalEventParameters(entity),
Line 68: new
QueryIdResolver<Long>(VdcQueryType.GetAuditLogById,
GetAuditLogByIdParameters.class));
Line 69: }
Line 69: }
Line 70:
Line 71: @Override
Line 72: protected Response performRemove(String id) {
Line 73: return performAction(VdcActionType.RemoveExternalEvent, new
RemoveExternalEventParameters(Long.valueOf(id)));
please implement asLong() method instead of Long.valueOf(id), see asGuid(id)
for details.
Line 74: }
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/EventMapper.java
Line 85: return model;
Line 86: }
Line 87:
Line 88: @Mapping(from = Event.class, to = AuditLog.class)
Line 89: public static AuditLog map(Event event, AuditLog entity) {
- please use isSetX() instead of event.getX != null
Line 90: AuditLog auditLog = (entity != null) ? entity : new AuditLog();
Line 91: auditLog.setseverity(map(event.getSeverity(), null));
Line 92: auditLog.setlog_time(event.getTime() == null ? new
Date((Calendar.getInstance().getTimeInMillis()))
Line 93: : event.getTime().toGregorianCalendar().getTime());
Line 122: }
Line 123: if (StringUtils.isNotEmpty(event.getCorrelationId())) {
Line 124: auditLog.setCorrelationId(event.getCorrelationId());
Line 125: }
Line 126: if (StringUtils.isNotEmpty(event.getCorrelationId())) {
you already have this setter
Line 127: auditLog.setCorrelationId(event.getCorrelationId());
Line 128: }
Line 129: if (StringUtils.isNotEmpty(event.getOrigin())) {
Line 130: auditLog.setOrigin(event.getOrigin());
--
To view, visit http://gerrit.ovirt.org/10097
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibdaf39df28c5f87db4c1bf9b5807c45a0a8ea9a8
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Ori Liel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
