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

Simplify initializing and running the Worker #114

Closed
andrewgy8 opened this issue Nov 1, 2019 · 4 comments
Closed

Simplify initializing and running the Worker #114

andrewgy8 opened this issue Nov 1, 2019 · 4 comments
Labels
enhancement New feature or request

Comments

@andrewgy8
Copy link
Contributor

One consistent peice of feedback that I have received is simplifying the Worker class. Right now, you must Initialize with the subs, each individual config attribute, run setup, run start, and then sleep.

Like this:

worker = Worker(
    [photo_uploaded],
    config.gc_project_id,
    config.credentials,
    config.ack_deadline,
)
worker.setup()
worker.start()
sleep(120)

I propose we simplify the API to run a worker to look something like this instead:

worker = Worker([photo_uploaded], config)
worker.run(sleep=120)

run would call both setup and start, and we could add the standard sleep method. In addition we consolidate the configuration into one attribute.

This would be backwards compatible since we will be creating a new method. And the change to Worker initialization could also fall back to the declared attributes if defined. Otherwise, use the config object.

@andrewgy8 andrewgy8 added the enhancement New feature or request label Nov 4, 2019
@tobami
Copy link
Contributor

tobami commented Nov 4, 2019

Sounds like a great plan to me!

@craigmulligan
Copy link
Contributor

Not sure this is convenient or possible but If you already have an api for setting up rele singleton with the required config, can't the worker use that api too? Then the setup for publishing and the Worker would be the same?

eg

# config.setup should always be called first.
rele.config.setup({
    'GC_CREDENTIALS': service_account.Credentials.from_service_account_file(
        GOOGLE_APPLICATION_CREDENTIALS
    ),
    'GC_PROJECT_ID': GOOGLE_PROJECT_ID,
})

worker = rele.Worker(
    [photo_uploaded],
)
worker.setup()
worker.start()
sleep(120)

@tobami
Copy link
Contributor

tobami commented Nov 19, 2019

That's certainly an option! We could make the config parameters optional in case the config was already initialized

craigmulligan pushed a commit to team-blaze/rele that referenced this issue Dec 19, 2019
This relates to mercadona#114 & mercadona#119

This makes makes all config variables nullable falling back to standard
google envars, without breaking the current api.

The new apis would look like this is you have
`GOOGLE_APPLICATION_CREDENTIALS` set.

```
rele.config.setup()
```

```
w = Worker([sub1, sub2])
w.run_forever()
```

TBH, I think reading global configs from the environment is easier to
reason about than a singleton so I'd suggest that. Any non globals should
just be passed into the instances on __init__.
tobami pushed a commit that referenced this issue Dec 20, 2019
This relates to #114 & #119

This makes makes all config variables nullable falling back to standard
google envars, without breaking the current api.
@andrewgy8
Copy link
Contributor Author

Released in #131

csaroff pushed a commit to csaroff/rele that referenced this issue Apr 19, 2023
This relates to mercadona#114 & mercadona#119

This makes makes all config variables nullable falling back to standard
google envars, without breaking the current api.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants