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

Connection cache for producer #46

Merged
merged 2 commits into from
May 4, 2014
Merged

Conversation

iproctor
Copy link
Collaborator

@iproctor iproctor commented May 3, 2014

Optionally globally cache connections to a host:port.

Connecting/reconnecting was broken into a separate Connection object. This is what is cached. Many Producers can share one Connection. The original event generation behavior was preserved.

The producer test runs twice, with and without caching. New tests were added with a mock socket to test edge cases.

@Raynos
Copy link
Collaborator

Raynos commented May 3, 2014

The travis build failed because node 0.9 doesn't exist on travis. It works fine in 0.8 and 0.10

@cainus
Copy link
Owner

cainus commented May 3, 2014

This is some awesome work. I've got a bunch of minor concerns:

  • Did you run up against this issue on 0.11?
  • I'm generally not a huge fan of soft-coded dependency versions relying on semver adherence. I've just been burned too many times by bad adherence breaking stuff. Any issue with hard-coding your readable-stream version?
  • In your production use, have you seen this?
  • It'd be ideal to get that travis build passing before merge too. Feel free to change the .travis.yml to have 0.11 instead of 0.9.

Again, just minor stuff. I really appreciate all this awesome work.

@cainus
Copy link
Owner

cainus commented May 3, 2014

It turns out that that buffermaker issue is a legit issue on 0.11 that bignum doesn't have a fix for yet. So I'm considering swapping out bignum for long or bignumber. Do you have a preference? Do you want to check performance of these first?

@@ -29,7 +29,8 @@
"binary": "0.3.0",
"crc32": "0.2.2",
"buffer-crc32": "0.2.1",
"bignum": "0.6.2"
"bignum": "0.6.2",
"readable-stream": "1.0.x"
Copy link
Collaborator

Choose a reason for hiding this comment

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

set this to 1.0.26 or something.

@Raynos
Copy link
Collaborator

Raynos commented May 3, 2014

@cainus we dont run 0.11 in production.

I think supporting 0.11 is out of scope for this PR.

We will look into 0.11 support once 0.12.0 is released.

The package.json thing is a good idea, we'll fix that.

cainus added a commit that referenced this pull request May 4, 2014
Connection cache for producer
@cainus cainus merged commit 7b3d59c into cainus:master May 4, 2014
@cainus
Copy link
Owner

cainus commented May 4, 2014

Alright... I've merged this in. I have a bit of upkeep to take care of before I publish, but that'll be in the next day or so.

And hey... a README update would be fantastic as well, if you're up for it.

@Raynos
Copy link
Collaborator

Raynos commented May 4, 2014

@cainus updated the README.

If you can publish a new version soon that would be great :)

@cainus
Copy link
Owner

cainus commented May 5, 2014

Alright... published as 0.7.0! Thanks guys! I added you as contributors on the repo as well.

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.

4 participants