add rpc #24
add rpc #24
Conversation
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ApiErrorResponse' | ||
| '404': |
There was a problem hiding this comment.
We also want to return different user/application side status code for:
- Global Attribute operation failure like unique key conflict or column doesn't exists etc. I have used 424 for this in start API.
- RPC execution error from worker : Maybe 412 (expectation failure) for this one
There was a problem hiding this comment.
Expectation failutre seems to be related to the Expect field in the header.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/417
But I didn't find another better code, so will use this one for now.
api-schema/xdb.yaml
Outdated
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/ProcessRpcRequest' |
There was a problem hiding this comment.
This name is hard to distinguish with the other two: ProcessExecutionRpcRequest and ProcessExecutionRpcResponse.
How about ProcessRpcWorkerRequest and ProcessRpcWorkerResponse?
api-schema/xdb.yaml
Outdated
| $ref: '#/components/schemas/LoadGlobalAttributeResponse' | ||
| ProcessRpcResponse: | ||
| type: object | ||
| description: the output of the rpc API |
There was a problem hiding this comment.
nit: the response of the worker RPC API
|
Or maybe it's better use 424 for both, and differentiate them in API Error model. Because I am worried that 412/417 will be confusing |
Agree. I also feel 424 is better than 412/417. |
| schema: | ||
| $ref: '#/components/schemas/ApiErrorResponse' | ||
| '424': | ||
| description: global attributes write failure / worker RPC execution failure |
There was a problem hiding this comment.
With this direction(I like it), I think we need to add a enum to tell which error type it is, like in iWF: https://github.com/indeedeng/iwf-idl/blob/af1891eb032c534b0b75894c611ead7e2954777d/iwf.yaml#L612
Otherwise, SDK won't be able to differentiate the two types of error.
There was a problem hiding this comment.
Maybe SDK can just throw the error messages with the same status code?
There was a problem hiding this comment.
It's not about message. User may need to get status code for different handling.
E.g. for global attr DB operation error, there should be a Postgres error code to use, in addition to the error message.
for error from worker execution, it may just be the http status code.
When you are doing that, I think it's better to rename
originalWorkerErrorStatus to originalWorkerErrorCode, and change from integer to string, so that it can be more generic.
There was a problem hiding this comment.
I am confused about the content you are talking, because I am thinking global attributes updating error is not a worker side error, it's server side.
No description provided.