ga4gh / task-execution-schemas

Apache License 2.0
82 stars 29 forks source link

Rename Job -> Task. Also, proof read and clean up. #39

Closed buchanae closed 7 years ago

buchanae commented 7 years ago

I did a thorough pass over the protobuf to proofread and clean things up. Most of this is documentation and style changes, but there are some bigger name changes as well.

One thing was to rename message fields to follow the protobuf style guide: https://developers.google.com/protocol-buffers/docs/style

The big change was to rename Job to Task as follows:

Task -> CreateTaskRequest
Job -> Task
RunTask() -> CreateTask()

The argument is: 1) Job vs Task has confused just about everyone at some point 2) This is the Task execution schema, after all 3) This allows the service endpoints to better follow Google's API guide 4) The result is actually pretty nice, IMO

One concern was that CreateTaskRequest would be too verbose. In reality, at least for Funnel, this will show up once or twice in the codebase, and will immediately be assigned to a shorter variable name, e.g. req := CreateTaskRequest{}, so I don't think brevity is worth the cost of confusion.

The Google API Design Guide inspired some of the changes here, because it is a clear and very well thought out convention, with a lot of experience behind it. https://cloud.google.com/apis/design/

Some other renames: JobLog -> ExecutorLog FileLog -> OutputFileLog package ga4gh_task_exec -> package tes JobListRequest -> ListTasksRequest ServiceInfoResponse -> ServiceInfo GetTask() returns a Task CreateTask() returns a Task CancelTask() uses HTTP POST instead of DELETE The HTTP endpoints for GetServiceInfo and CancelTask changed

geoffjentry commented 7 years ago

👍

buchanae commented 7 years ago

I've added some more (fairly minor) cleanup:

buchanae commented 7 years ago

Oops, I didn't look closely enough at TimeOfDay. It's only a time, not a date. Seems obvious now.

Would people prefer a string in ISO 8601, or a struct with date and time fields?

geoffjentry commented 7 years ago

@buchanae I have a weak preference towards ISO8601. While a struct is more flexible I think it's hard for someone to concretely argue against ISO8601 and from a practical perspective I think most people can or already are going to be using that.

buchanae commented 7 years ago

Tacking on a few more minor changes here:

1) FileType is an enum 2) Add a comment to the PAUSED state, to the effect that it is allowed but not required to be supported. 3) Namespaced the service info HTTP endpoint to "/v1/tasks/service-info"

buchanae commented 7 years ago

@geoffjentry Would RFC 3339 be preferable and/or correct? As I understand, it's a profile (subset?) of ISO 8601, and represents "an instant in time" which seems appropriate here, but I'm not an expert on the details.

geoffjentry commented 7 years ago

@buchanae I'm far from expert here but you're right - ISO8601 has multiple possible representations and that's not generally what people mean :) I think you're right here.