Skip to content
This repository has been archived by the owner on Sep 20, 2022. It is now read-only.

Use an interface for jobs instead of a non-type-safe handler function (would break backwards-compatibility) #14

Open
albrow opened this issue May 15, 2015 · 4 comments

Comments

@albrow
Copy link
Owner

albrow commented May 15, 2015

I would like to change the public API in a pretty major way by using an interface for jobs instead of a handler function (which is not typesafe). The current implementation feels a little bit messy and unidiomatic to me.

Jobs follows semantic versioning, which means breaking changes are fair game until version 1.0. However, I never put a warning in the README about this, so I wanted to make sure it was okay with everyone before making this breaking change. I would also be happy to hear feedback on the approach.

The basic idea is to create an interface that might look something like this:

type Job interface {
    Execute() error
    JobId() string
    SetJobId(string)
    JobStatus() Status
    SetJobStatus(Status)
}

Most of these have straightforward implementations which could be covered with an embeddable DefaultJob or JobData type, similar to the approach I use in zoom.

type DefaultJob struct {
    Id string
    Status Status
}

func (j DefaultJob) JobId() string {
    return j.Id
}

func (j DefaultJob) SetJobId(id string) {
    j.Id = id
}

// etc for other getters and setters

So job type declarations would now look like this:

type EmailJob struct {
    User *model.User
    jobs.DefaultJob
}

func (j EmailJob) Execute() error {
    msg := fmt.Sprintf("Hello, %s! Thanks for signing up for foo.com.", user.Name)
    if err := emails.Send(j.User.EmailAddress, msg); err != nil {
        return err
    }
}

I can leverage zoom as a library to easily serialize all the exported fields in any job struct. So when a job gets retrieved from the database, all the struct fields will be filled in. Then the worker will just call the Execute method.

There are a few advantages to this approach:

  1. Type-safety: If you don't embed a DefaultJob or provide your own implementations of the methods needed, or if you don't define an Execute method, the compiler will tell you, whereas previously these types of omissions would be runtime errors. Workers can also execute jobs by calling the Execute method instead of jumping through fiery hoops with reflection.
  2. Flexibility: The Execute function can safely access any exported properties of the job type, so in effect this solves the multiple argument problem.
  3. Idiomaticness: Using an empty interface as an argument to RegisterJob just feels wrong.

Let me know what you think. If I don't hear any objections I'll plan on converting to the new implementation sometime in the coming weeks.

@epelc
Copy link
Contributor

epelc commented May 15, 2015

+1 I like this idea. I was also turned off by HandlerFunc being an interface{} which required reflection.

Also what do you think about exposing job details to the Execute func? For example the job id and name might be useful in some cases(ie smartly rescheduling the current job based on the results it got).

@albrow
Copy link
Owner Author

albrow commented May 15, 2015

@epelc thanks for the feedback!

With the implementation described above, the job details are already exposed in the Execute method because DefaultJob is embedded in EmailJob. So if you needed access to the id, status, etc.:

func (j EmailJob) Execute() error {
    id = j.JobId()
    // ...
}

@sogko
Copy link

sogko commented Jul 18, 2015

+1 this proposal as well, very idiomatic and feels right

@cdrage
Copy link
Contributor

cdrage commented Dec 30, 2015

+1, finding it difficult to implement with current examples / ideology on the README

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

No branches or pull requests

4 participants