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

producer: Adds a Ping method in order to validate an nsqd connection #119

Merged
merged 1 commit into from
Feb 23, 2015

Conversation

dcarney
Copy link
Contributor

@dcarney dcarney commented Feb 18, 2015

Adds a Ping method to the Producer, which allows for testing a Producer's nsqd connection w/o publishing any messages to a topic.

I've found it to be useful for failing fast when configuring Producers that might not Publish immediately (since otherwise, the connections are only made lazily on Publish).

Not thrilled with the method name "Ping", so am totally open to other suggestions.

@mreiferson
Copy link
Member

My only comment is - let's actually make this send a NOP command to the nsqd?

@mreiferson
Copy link
Member

works for me - do you mind squashing down to one commit?

@jehiah any thoughts before we merge?

@jehiah
Copy link
Member

jehiah commented Feb 21, 2015

👍

@dcarney dcarney force-pushed the producer-adds-ping-method branch from 8109ec4 to ccc03f1 Compare February 22, 2015 17:07
@dcarney
Copy link
Contributor Author

dcarney commented Feb 22, 2015

@mreiferson Squashed it. Thanks!

@mreiferson
Copy link
Member

hmm, looks like some other recently merged code is conflicting - needs a rebase now, sorry!

@dcarney dcarney force-pushed the producer-adds-ping-method branch from ccc03f1 to a49cb28 Compare February 23, 2015 00:47
@dcarney
Copy link
Contributor Author

dcarney commented Feb 23, 2015

No problem. Rebased it, and also removed an unecessary "fmt" import.

jehiah added a commit that referenced this pull request Feb 23, 2015
producer: Adds a Ping method in order to validate an nsqd connection
@jehiah jehiah merged commit 596f983 into nsqio:master Feb 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants