-
Notifications
You must be signed in to change notification settings - Fork 3
Add basic pollers #9
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
Conversation
from typing import TypedDict | ||
|
||
|
||
class WorkerOptions(TypedDict, total=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would want to expose WorkerOptions to client like the go client here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're included in the init.py so they can be imported like:
from cadence.worker import WorkerOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not expect customer to search for WorkerOptions definition in _types package, we should define those in some public files so that it's easy for customer to understand it's usage.
cadence/worker/_poller.py
Outdated
self._permits = permits | ||
self._poll = poll | ||
self._executor = executor | ||
self._background_tasks = set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: set[async.ioFuture]()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
cadence/worker/_poller.py
Outdated
return | ||
|
||
# Need to store a reference to the async task or it may be garbage collected | ||
scheduled = asyncio.create_task(self._execute_task(task)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventloop has run_in_executor api.
scheduled = asyncio.create_task(self._execute_task(task)) | |
scheduled = asyncio.get_running_loop().run_in_executor(self._executor, self._execute_task, task) |
or you can set default executor in Poller init and thus omit the first argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline and renamed. The callback will ultimately dispatch it via another component.
cf495f2
to
6daae1a
Compare
cadence/worker/_activity.py
Outdated
async def run(self): | ||
await self._poller.run() | ||
|
||
async def poll(self) -> Optional[PollForActivityTaskResponse]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
async def poll(self) -> Optional[PollForActivityTaskResponse]: | |
async def _poll(self) -> Optional[PollForActivityTaskResponse]: |
cadence/worker/_activity.py
Outdated
else: | ||
return None | ||
|
||
async def execute(self, task: PollForActivityTaskResponse): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
async def execute(self, task: PollForActivityTaskResponse): | |
async def _execute(self, task: PollForActivityTaskResponse): |
What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes