Michael Pasternak has posted comments on this change.
Change subject: restapi: RFE-Adding External Tasks Support
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(21 inline comments)
....................................................
File
backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/QueryHelper.java
Line 157: } else {
Line 158: return null;
Line 159: }
Line 160: }
Line 161:
these changes are not relevant to the current feature
Line 162: public static boolean hasConstraint(MultivaluedMap<String,
String> queries, String constraint) {
Line 163: return queries != null && queries.containsKey(constraint) ?
true : false;
Line 164: }
Line 165:
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/JobResource.java
Line 38: @Formatted
Line 39: public Job get();
Line 40:
Line 41: @Path("steps")
Line 42: public StepsResource getStepsResource();
please format this file
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/StepResource.java
Line 27: public Response end(Action action);
Line 28:
Line 29: @GET
Line 30: @Formatted
Line 31: public Step get();
please format this file
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 586: <xs:element ref="pm_proxy_types" minOccurs="0"/>
Line 587: <xs:element ref="cpu_modes" minOccurs="0"/>
Line 588: <xs:element ref="sgio_options" minOccurs="0"/>
Line 589: <!-- External Tasks -->
Line 590: <xs:element ref="step_enum_types" minOccurs="0"/>
step_types (we do not expose any ENUMs in the api)
Line 591: <!-- Gluster related -->
Line 592: <xs:element ref="gluster_volume_types" minOccurs="0"/>
Line 593: <xs:element ref="transport_types" minOccurs="0"/>
Line 594: <xs:element ref="gluster_volume_states" minOccurs="0"/>
Line 3163:
Line 3164: <xs:element name="steps" type="Steps"/>
Line 3165: <xs:element name="step" type="Step"/>
Line 3166:
Line 3167: <xs:element name="step_enum_types" type="StepEnumTypes"/>
step_types (we do not expose any ENUMs in the api)
Line 3168:
Line 3169: <xs:complexType name="StepEnumTypes">
Line 3170: <xs:sequence>
Line 3171: <xs:element name="step_enum_type" type="xs:string"
minOccurs="0" maxOccurs="unbounded">
Line 3165: <xs:element name="step" type="Step"/>
Line 3166:
Line 3167: <xs:element name="step_enum_types" type="StepEnumTypes"/>
Line 3168:
Line 3169: <xs:complexType name="StepEnumTypes">
StepTypes (we do not expose any ENUMs in the api)
Line 3170: <xs:sequence>
Line 3171: <xs:element name="step_enum_type" type="xs:string"
minOccurs="0" maxOccurs="unbounded">
Line 3172: <xs:annotation>
Line 3173: <xs:appinfo>
Line 3167: <xs:element name="step_enum_types" type="StepEnumTypes"/>
Line 3168:
Line 3169: <xs:complexType name="StepEnumTypes">
Line 3170: <xs:sequence>
Line 3171: <xs:element name="step_enum_type" type="xs:string"
minOccurs="0" maxOccurs="unbounded">
same
Line 3172: <xs:annotation>
Line 3173: <xs:appinfo>
Line 3174: <jaxb:property name="StepEnumTypes"/>
Line 3175: </xs:appinfo>
Line 3185: <xs:enumeration value="EXECUTING"/>
Line 3186: <xs:enumeration value="FINALIZING"/>
Line 3187: <xs:enumeration value="UNKNOWN"/>
Line 3188: </xs:restriction>
Line 3189: </xs:simpleType>
please move it to the org.ovirt.engine.api.model as java enum, we do not expose
any enums in api, but strings,
please also create mapping public<-->backend enum (see StorageType)
Line 3190:
Line 3191: <xs:complexType name="Step">
Line 3192: <xs:annotation>
Line 3193: <xs:appinfo>
Line 3200: <xs:element name="parent_step" type="Step" minOccurs="0"
maxOccurs="1"/>
Line 3201: <xs:element name="job" type="Job" minOccurs="0"
maxOccurs="1"/>
Line 3202: <xs:element name="type" type="StepEnum" minOccurs="0"
maxOccurs="1"/>
Line 3203: <xs:element name="number" type="xs:int" minOccurs="0"
maxOccurs="1"/>
Line 3204: <xs:element name="status" type="Status" minOccurs="0"
maxOccurs="1"/>
please reuse "status" object here it uses cross api status.state|detail pattern
Line 3205: <xs:element name="start_time" type="xs:dateTime"
minOccurs="0" maxOccurs="1"/>
Line 3206: <xs:element name="end_time" type="xs:dateTime"
minOccurs="0" maxOccurs="1"/>
Line 3207: <xs:element name="external" type="xs:boolean"
minOccurs="0" maxOccurs="1"/>
Line 3208: </xs:sequence>
Line 3233: </xs:annotation>
Line 3234: <xs:complexContent>
Line 3235: <xs:extension base="BaseResource">
Line 3236: <xs:sequence>
Line 3237: <xs:element name="status" type="Status" minOccurs="0"
maxOccurs="1"/>
please reuse "status" object here it uses cross api status.state|detail pattern
Line 3238: <xs:element name="owner" type="User" minOccurs="0"
maxOccurs="1"/>
Line 3239: <xs:element name="start_time" type="xs:dateTime"
minOccurs="0" maxOccurs="1"/>
Line 3240: <xs:element name="end_time" type="xs:dateTime"
minOccurs="0" maxOccurs="1"/>
Line 3241: <xs:element name="last_updated" type="xs:dateTime"
minOccurs="0" maxOccurs="1"/>
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 3211: request:
Line 3212: body:
Line 3213: parameterType: Action
Line 3214: signatures:
Line 3215: - mandatoryArguments: {}
in the code i see you mandate status by:
validateParameters(action, "status")
so it's has to be in mandatory parms here
Line 3216: optionalArguments: {action.force: 'xs:boolean'}
Line 3217: urlparams: {}
Line 3218: headers:
Line 3219: Content-Type: {value: application/xml|json, required: true}
Line 3272: headers:
Line 3273: Content-Type: {value: application/xml|json, required: true}
Line 3274: Correlation-Id: {value: 'any string', required: false}
Line 3275:
Line 3276:
please s/status/status.state for all instances
Line 3277:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendResource.java
Line 48: private static final String NON_BLOCKING_EXPECTATION =
"202-accepted";
Line 49: protected static final Log LOG =
LogFactory.getLog(BackendResource.class);
Line 50: public static final String POPULATE = "All-Content";
Line 51: public static final String JOB_ID = "JobId";
Line 52: public static final String STEP_ID = "StepId";
makes sense to add _CONSTRAINT suffix to these two to comply with rest of the
constraints
Line 53: protected <T> T getEntity(Class<T> clz, SearchType searchType,
String constraint) {
Line 54: try {
Line 55: VdcQueryReturnValue result = runQuery(VdcQueryType.Search,
Line 56: new
SearchParameters(constraint, searchType));
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStepsResource.java
Line 37: }
Line 38:
Line 39: @Override
Line 40: public Response add(Step step) {
Line 41: validateParameters(step, "Type", "status" , "description");
s/Type/type
s/status/status.state
Line 42: String id;
Line 43: if (step.isSetParentStep()) {
Line 44: id = step.getParentStep().getId();
Line 45: }
Line 40: public Response add(Step step) {
Line 41: validateParameters(step, "Type", "status" , "description");
Line 42: String id;
Line 43: if (step.isSetParentStep()) {
Line 44: id = step.getParentStep().getId();
are you ready for NULL here? cause step.getParentStep().getId() can return
NULL, i'd suggest validating id in this context
Line 45: }
Line 46: else {
Line 47: id = jobId.toString();
Line 48: }
Line 47: id = jobId.toString();
Line 48: }
Line 49:
Line 50: return performCreate(VdcActionType.AddExternalStep,
Line 51: new
AddExternalStepParameters(Guid.createGuidFromString(id),
step.getDescription(),StepMapper.map(step.getType()),
JobMapper.map(step.getStatus(), null)),
use asGuid() instead of Guid.createGuidFromString(id) as it throws format
exception while asGuid() BAD_REQUEST
Line 52: new
QueryIdResolver<Guid>(VdcQueryType.GetStepByStepId, IdQueryParameters.class));
Line 53: }
Line 54:
Line 55: @Override
Line 51: new
AddExternalStepParameters(Guid.createGuidFromString(id),
step.getDescription(),StepMapper.map(step.getType()),
JobMapper.map(step.getStatus(), null)),
Line 52: new
QueryIdResolver<Guid>(VdcQueryType.GetStepByStepId, IdQueryParameters.class));
Line 53: }
Line 54:
Line 55: @Override
please add here @SingleEntityResource annotation here, otherwise no action will
be able to fetch updated resource upon competition
Line 56: public StepResource getStepSubResource(@PathParam("id") String id)
{
Line 57: return inject(new BackendStepResource(id, this));
Line 58: }
Line 59:
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/JobMapper.java
Line 40: public static org.ovirt.engine.core.common.job.Job map(Job job,
Line 41: org.ovirt.engine.core.common.job.Job entity) {
Line 42: org.ovirt.engine.core.common.job.Job target =
Line 43: entity != null ? entity : new
org.ovirt.engine.core.common.job.Job();
Line 44: target.setId(new Guid(job.getId()));
use GuidUtils.asGuid() as it formats BAD_REQUEST on format exception, while new
Guid() just throws runtime exception
Line 45: if (job.isSetDescription()) {
Line 46: target.setDescription(job.getDescription());
Line 47: }
Line 48: if (job.isSetStatus()) {
Line 48: if (job.isSetStatus()) {
Line 49: target.setStatus(map(job.getStatus(), null));
Line 50: }
Line 51: if (job.isSetOwner()) {
Line 52: target.setOwnerId(new Guid(job.getOwner().getId()));
use GuidUtils.asGuid() as it formats BAD_REQUEST on format exception, while new
Guid() just throws runtime exception
Line 53: }
Line 54: target.setStartTime(job.isSetStartTime() ?
job.getStartTime().toGregorianCalendar().getTime()
Line 55: : new
Date((Calendar.getInstance().getTimeInMillis())));
Line 56: target.setEndTime(job.isSetEndTime() ?
job.getEndTime().toGregorianCalendar().getTime()
Line 105: return st;
Line 106: }
Line 107: st.setState(JobExecutionStatus.UNKNOWN.name());
Line 108: return st;
Line 109: }
use .value() here instead of .name() - it will return name().toLower()
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/StepMapper.java
Line 41: public static org.ovirt.engine.core.common.job.Step map(Step step,
Line 42: org.ovirt.engine.core.common.job.Step entity) {
Line 43: org.ovirt.engine.core.common.job.Step target =
Line 44: entity != null ? entity : new
org.ovirt.engine.core.common.job.Step();
Line 45: target.setId(new Guid(step.getId()));
use GuidUtils.asGuid() as it formats BAD_REQUEST on format exception, while new
Guid() just throws runtime exception
Line 46: if (step.isSetParentStep()) {
Line 47:
target.setParentStepId(Guid.createGuidFromString(step.getParentStep().getId()));
Line 48: }
Line 49:
target.setJobId(Guid.createGuidFromString(step.getJob().getId()));
--
To view, visit http://gerrit.ovirt.org/16159
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1b95a094dc586e6ebbdacd44e0a034e91601952
Gerrit-PatchSet: 4
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]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
