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

Pipeline vhosts #410

Merged
merged 8 commits into from Feb 26, 2017
Merged

Pipeline vhosts #410

merged 8 commits into from Feb 26, 2017

Conversation

mattbennett
Copy link
Member

@mattbennett mattbennett commented Feb 13, 2017

No description provided.

@davidszotten
Copy link
Member

could you rebase this on top of master please?

@@ -140,3 +140,65 @@ def find_free_port(host='127.0.0.1'):
port = sock.getsockname()[1]
sock.close()
return port


class ResourcePipeline(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we see this being re-used? (if not, maybe move to conftest?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to use it in nameko-sqlalchemy to speed up creation of database schemas.

def get(self):
return self.ready.get()

def discard(self, vhost):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're keeping this generic, maybe vhost -> resource or similar

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, thanks

while self.ready.qsize():
unused = self.ready.get()
self.trash.put(unused)
eventlet.sleep() # allow create thread to run
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come we need this? (is it not enough to wait in wait below?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for the case where the create loop is blocked waiting to put an item into the ready queue (i.e. waiting for it to be less than maxsize).

unused = self.ready.get() unblocks the create thread but doesn't yield, so we exit the while loop here before giving it a chance to run. Once unblocked, the create thread deposits its item into the ready queue, and if we've already exited the loop, that final item isn't destroyed.

I've expanded the inline comment to hopefully explain

)
rabbit_manager.create_vhost(vhost)
rabbit_manager.set_vhost_permissions(vhost, username, '.*', '.*', '.*')
vhost = vhost_pipeline.get()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought on using a contextmanager interface for get and discard as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice idea

STOP = object()

def __init__(self, create, destroy, size=3):
if size == 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did you pick the size?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size=3? From trial and error in the vhost usage -- 3 resulted in the best speedup. It's a balance between having something available when you need it and the "waste" of destroying what's left unused at the end of the run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so do you think 3 is still a reasonable default for other use-cases? (or do we e.g. want to force the user to choose?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer having a default rather than forcing the user to select something. I think 3 is fine.

# allow create thread to run if it is blocked waiting for capacity
# in the ready queue. otherwise we'll exit this loop before the
# final item is added to the ready queue and it won't be destroyed
eventlet.sleep()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm still thinking about this but we might have a race here (e.g. if create is slow). may need e.g. a semaphore around the body of the while look in _create

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. I'll rework it.

@mattbennett mattbennett merged commit 7ac629c into nameko:master Feb 26, 2017
@mattbennett mattbennett deleted the pipeline-vhosts branch February 26, 2017 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants