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

[Proposal] Change Payload to bytes & Add Wrapper around Key-Value pair payload #109

Closed
hibiken opened this issue Mar 11, 2020 · 11 comments · Fixed by #287
Closed

[Proposal] Change Payload to bytes & Add Wrapper around Key-Value pair payload #109

hibiken opened this issue Mar 11, 2020 · 11 comments · Fixed by #287
Assignees
Labels
enhancement New feature or request idea Feature Suggestion or Idea

Comments

@hibiken
Copy link
Owner

hibiken commented Mar 11, 2020

Is your feature request related to a problem? Please describe.
I'm always frustrated when I need to call GetX method for all the values in Payload to get all the data (also need to make sure that I don't make a typo in payload key string).
It'd be nice if asynq supported en/decoding protobuf or gob message directly so that I can load that data to an object with one method call.

Describe the solution you'd like
initial proposal:

type EmailTaskPayload struct {
    UserID int
}

// client side.
func main() {
     c := asynq.NewClient(asynq.RedisClientOpt{Addr: ":6379"})

    t := asynq.NewTask("email:welcome", EmailTaskPayload{UserID: 42})
    err := c.Enqueue(t)
    if err != nil {
        log.Fatalf("could not enqueue task: %v", err)
    }
}

// background workers side
func main() {
    bg := asynq.NewBackground(r, &asynq.Config{
        Concurrency: 10,
    })

    mux := asynq.NewMux()
    mux.HandleFunc("email:welcome", emailHandler)

    bg.Run(mux)
}

func emailHandler(ctx context.Context, t *asynq.Task) error {
    var p EmailTaskPayload
    err := t.ReadPayload(&p)
    if err != nil {
        return err
    }
    fmt.Printf("Send Email to User %d\n", p.UserID)
    return nil
}

Describe alternatives you've considered
None for now.

Additional context
None for now.

@hibiken hibiken added the enhancement New feature or request label Mar 11, 2020
@hibiken hibiken self-assigned this Mar 11, 2020
@hibiken hibiken added the idea Feature Suggestion or Idea label Mar 11, 2020
@hibiken hibiken changed the title [FEATURE REQUEST] Add support for Gob, Protobuf, etc for encoding payload [FEATURE REQUEST] Add support for Gob for encoding payload Mar 11, 2020
@hibiken hibiken changed the title [FEATURE REQUEST] Add support for Gob for encoding payload [FEATURE REQUEST] Use Gob for encoding payload Mar 11, 2020
@hibiken hibiken changed the title [FEATURE REQUEST] Use Gob for encoding payload [Proposal] API Changes & Use Gob for encoding payload Mar 13, 2020
@hibiken
Copy link
Owner Author

hibiken commented Mar 13, 2020

Closing this for now.

@hibiken hibiken closed this as completed Mar 13, 2020
@andygrunwald
Copy link

Hey @hibiken,
coming back to this, because I am running the same problem here.
Would love to store []byte in the message itself.

What was your reasoning for closing this?

@hibiken
Copy link
Owner Author

hibiken commented Feb 22, 2021

Hey @andygrunwald thanks for your comment!
I'm actually considering changing the signature of NewTask to

func NewTask(typename string, payload []byte) *Task

and add a nice wrapper around the payload to make it easier to do key-value pairs like the current API supports.

something like:

t := NewTask("mytask", asynq.KV(map[string]interface{}{"mykey": 123}))

I'll open this issue again so that it's more visible to people.

In the meantime, is it possible to workaround this by storing the bytes as string in the payload?

@hibiken hibiken reopened this Feb 22, 2021
@hibiken hibiken changed the title [Proposal] API Changes & Use Gob for encoding payload [Proposal] Change Payload to bytes & Add Wrapper around Key-Value pair payload Feb 22, 2021
@andygrunwald
Copy link

Thanks @hibiken.
I thought about something similar.

In the meantime, is it possible to workaround this by storing the bytes as string in the payload?

Didn't check this yet. Need to do another test.
However, I am also running into this issue:

Screen Shot 2021-02-22 at 19 38 05

I think the []byte payload is able to solve both.

@andygrunwald
Copy link

Related to #245

@hibiken
Copy link
Owner Author

hibiken commented Feb 23, 2021

@andygrunwald Would you mind describing the issue you're seeing?
I want to understand your use case so that I can take that into account when I make API change to support storing payload as []byte.

@disc
Copy link
Contributor

disc commented Mar 11, 2021

Hello @hibiken

I have the same issue when consumer tried to execute task with this payload:

WARN: Could not remove task id=f83cca71-620b-45ff-a309-c6917dd2f248 type="app.event" from "asynq:{app.event}:active" err: NOT FOUND; Will retry syncing

Payload:

{"eventType": "Signup", "countryCode": "au"}

The full message is:

{"Type":"app.event","Payload":{"countryCode":"au","eventType":"Signup"},"ID":"5663097f-6cb7-4464-b1d2-b6a8c5a59bb7","Queue":"app.event","Retry":25,"Retried":0,"ErrorMsg":"","Timeout":1800,"Deadline":0,"UniqueKey":""}

If I change key from eventType to abc in the payload everything is ok and no errors occur.

{"abc": "Signup", "countryCode": "au"}

Is there any way to fix it?

@disc
Copy link
Contributor

disc commented Mar 11, 2021

^^ UPD:

I found the difference, the keys of object sorted in different way, looks like expected payload sorted alphabetically.

Payload in redis queue asynq:{app.event}:active

"Payload":{"eventType":"Signup","countryCode":"au"}"

Payload in consumer

"Payload":{"countryCode":"au","eventType":"Signup"}"

The problem related to php producer, json_encode method does not work as json.Marshal in golang

@hibiken
Copy link
Owner Author

hibiken commented Mar 11, 2021

@disc Thanks for updating this issue.
Just to clarify, you're enqueueing a task using PHP client? Not using asynq.Client?

FYI: This issue should go away in the next release. I'm currently working on redis key re-design so that we only need to move IDs around and don't need to use JSON data to remove entry from redis sets.

@disc
Copy link
Contributor

disc commented Mar 11, 2021

@disc Thanks for updating this issue.
Just to clarify, you're enqueueing a task using PHP client? Not using asynq.Client?

Yes, I wrote PHP client for asynq, it just produces a message to the redis queue as the asynq worker expects

@hibiken
Copy link
Owner Author

hibiken commented Mar 11, 2021

That's great! I should probably document the message schema in Wiki so that people can write clients in other languages :)

Related: #105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request idea Feature Suggestion or Idea
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants