Skip to content

Conversation

@bakins
Copy link
Member

@bakins bakins commented Jul 3, 2012

A little rough. Adds an options table to connect() with a pool option per issue 133.

@agentzh
Copy link
Member

agentzh commented Jul 3, 2012

Thanks! I'll look into your patch tomorrow :)

@agentzh
Copy link
Member

agentzh commented Jul 3, 2012

Some questions/suggestions:

  1. Could you please add some test cases for this new feature (including the cases for both TCP sockets and unix sockets)?
  2. It seems that your patch disallow option tables for unix domain sockets?
  3. I hope that the "pool" option is the whole key for distinguishing connection pools, that is, we should not prepend "host:port" or "path" to the "pool" option value such that the user can choose to make several different backends to share a single pool if she wishes.

Thanks!
-agentzh

@bakins
Copy link
Member Author

bakins commented Jul 3, 2012

Sure. I hacked that together in between meetings, so I know it's pretty rough.

on number 3 -- good point, I didn't do it that way but that makes more sense. We'd just need to document the behavior.

I may start from scratch as it's pretty ugly as is.

@bakins
Copy link
Member Author

bakins commented Jul 3, 2012

I probably need to squash down the commits. Added tests.

@agentzh
Copy link
Member

agentzh commented Jul 3, 2012

No worries, I'll commit a single diff rather than your original commits :)

@bakins
Copy link
Member Author

bakins commented Jul 3, 2012

I'm a commit early commit often sort of guy ;)

Tests passed. I need better tests, but unix sockets are working now. Took more hackery than I wanted. I'm sure the code can be cleaned up.

@agentzh
Copy link
Member

agentzh commented Jul 11, 2012

i've committed a modified version of your patch to the "socket-pool" branch:

1d8d157

Please have a look ;) Feedback welcome!

Thanks!
-agentzh

@bakins
Copy link
Member Author

bakins commented Jul 11, 2012

Looks good. I'll run some tests tomorrow.

You handled the table/key much better than I did.

@bakins bakins closed this Jul 12, 2012
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.

2 participants