Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task updates #111

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Task updates #111

wants to merge 3 commits into from

Conversation

eric
Copy link
Contributor

@eric eric commented Aug 4, 2014

WIP: This is a work-in-progress and is not ready to be merged yet.

Overview

Provides a way for clients to attach status updates to tasks they are working on. The contents of the status is an opaque blob that skuld has no understanding of.

The status updates are stored in the claim as a vector and it is up to the client to provide the index of the next available index in the logs vector.

While a client is working on a claim, no status update should use an index more than once. If it does, any of the updates for that index may end up winning, and there is no guarantee that it won't change in the future.

Fixes #18

@eric
Copy link
Contributor Author

eric commented Aug 4, 2014

@aphyr: In your explanation of the feature in #18 you suggested that the message be a byte array, which seemed very logical.

As I implemented this, I realized that with the HTTP interface, we will be getting a string back, which lead me to reconsider if it made the most sense for a log entry to be a byte array or a string.

It would be possible to require the message to be passed in the HTTP interface as a base64 encoded byte array, but it almost seems easier to just have it be a string and leave it up to the consumer if they want to base64 encode what is persisted.

It seems like this logic mostly needs to be decided in http/update!.

@eric
Copy link
Contributor Author

eric commented Aug 4, 2014

@aphyr: Is it worth putting something in place in task/update that rejects an update to an index where the index is not empty and the message does not match the existing message?

@aphyr
Copy link
Contributor

aphyr commented Aug 4, 2014

Definitely byte array, JSON is just gonna have to deal; base64 might be the right solution there.

Conflicting updates should error, I think, but it might be sensible to allow idempotent updates to succeed with an advisory "yeah, we saw that already" in case clients lose their conn and need to retry.

@eric
Copy link
Contributor Author

eric commented Aug 4, 2014

What about a query param that specifies the encoding of the message that can either be... base64 or utf8?

It seems sad to have to force a client to have a base64 encoder/decoder if they're just sending text.

@aphyr
Copy link
Contributor

aphyr commented Aug 4, 2014

Clients will have to support base64 either way, so I'm not sure adding complexity buys anything here.

@eric
Copy link
Contributor Author

eric commented Aug 5, 2014

Just realized a couple things:

  1. I can't find somewhere else that currently would require the client to support base64
  2. The current HTTP interface does not do anything specific with :data, so it is currently being accepted as a string and persisted

@aphyr
Copy link
Contributor

aphyr commented Aug 5, 2014

What are you going to do for HTTP clients that attempt to read a task whose payload is not valid UTF-8?

@eric
Copy link
Contributor Author

eric commented Aug 5, 2014

That's a good question I don't have an answer to.

I guess right now the answer is we'd throw a 500...

@aphyr
Copy link
Contributor

aphyr commented Aug 5, 2014

Yep. Best to just encode it. :)

@eric
Copy link
Contributor Author

eric commented Aug 5, 2014

I've had this terribly incomplete thought of being able to or wanting to be able to enqueue new tasks easily with curl, but I really don't know if it would be worth the trouble.

@aphyr
Copy link
Contributor

aphyr commented Aug 5, 2014

Yeah, JSON really needs a byte type. :-(

@aphyr
Copy link
Contributor

aphyr commented Aug 5, 2014

If you wanna add an option for it, that's fine, but the clients will all have to implement negotiation. I'd do it as an x- content-type and accept header, I think.

@lmarburger
Copy link

Do clients actually want to do content negotiation or do they just want to know the mime type of the status update?

@aphyr
Copy link
Contributor

aphyr commented Aug 5, 2014

Well at a minimum, you'll want to send varying content types for submission. Accept headers are the right way to negotiate representation formats for reads.

@aphyr
Copy link
Contributor

aphyr commented Aug 5, 2014

Oh, Larry, this is just for the top-level task representation, not for the update mime-type itself. That's an opaque blob.

@eric
Copy link
Contributor Author

eric commented Aug 5, 2014

I think I'm ready to give up on this curl thing for now — I am definitely worried I'm developing for something that doesn't actually exist.

@aphyr: do you think I should fix the marshaling of :data in this branch or a separate one?

@lmarburger
Copy link

@aphyr Interesting. I think I need to understand how the HTTP interface works first. I thought this was just a simple matter of using multipart/form-data.

@aphyr
Copy link
Contributor

aphyr commented Aug 5, 2014

A separate branch, I think. If you wanna do content negotiation I'm open to it! You'll just need a way to represent binary payloads where they exist.

@eric
Copy link
Contributor Author

eric commented Aug 9, 2014

It ends up that what I did for converting IDs into base64 was in the wrong place.

We need a way to handle IDs differently than other Bytes that need to be base64 encoded.

@aphyr: Do you have any suggestions for the best way to handle serializing the results?

We could either introduce a new type for ids and provide an encoder to add-encoder, or could pluck the id out of the result and serialize it manually.

@aphyr
Copy link
Contributor

aphyr commented Aug 11, 2014

Serialize before it hits the JSON encoder, perhaps?

@eric
Copy link
Contributor Author

eric commented Aug 11, 2014

That was what I ended up starting doing, but I wasn't sure if it was too ugly.

I'll keep going in that direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task updates
3 participants