-
Notifications
You must be signed in to change notification settings - Fork 133
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
Integrating Semaphores into Executor #31
Conversation
@@ -78,8 +78,9 @@ def cancel(self): | |||
|
|||
class TransferMeta(object): | |||
"""Holds metadata about the TransferFuture""" | |||
def __init__(self, call_args=None): | |||
def __init__(self, call_args=None, id=None): |
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.
This shadow's the built-in id.
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.
It should not be an issue because it is isolated to the class but I can update it to something more explicit.
I don't see any major issues, though I think @jamesls should have a look. |
Alright I updated the code based on feedback. Let me know what you think. |
""" | ||
self._semaphore = threading.Semaphore(count) | ||
|
||
def acquire(self, tag, blocking=True): |
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.
Probably worth documenting/commenting the reason there's params that are required but not used is because we want to be API compatible with the SlidingWindowSemaphore
.
|
This makes it easier to track the submission of tasks as it relates to a specific transfer request and makes it easier for tracking requests from a subscriber.
Refactored interface for BoundedExecutors to be less general.
Adds consistency with Task and avoids confusion with shadowing builtin id.
When result() is called there is no guarantee that the semaphore is released because it is part of a done callback. As a result, submission would sometimes happen before the semaphore being released causing random failures
I updated the docstring. @JordonPhillips let me know if you have anymore comments. |
⛵ |
In order to support the SlidingWindowSemaphore and fix #20, I decided to rip out the current implementation on how we bound the executors and replace it with a semaphore approach.
The PR builds off of: #30. You can look at that one or look at both of the PR's via this one.
The larger changes in this PR include:
cc @jamesls @JordonPhillips