Make Nsq::Discovery compatible with v1.0.0-compat responses

pacoguzman commented 7 years ago

This PR implements a solution for https://github.com/wistia/nsq-ruby/issues/37

Notes:

I had a spec failure locally, I'll try to fix it as soon as possible

.................F...............................................................

Failures:

  1) Nsq::Producer connecting via nsqlookupd #connections should be connected to all nsqds
     Failure/Error: wait_for{ @producer.connections.length == @cluster.nsqd.length }
     Timeout::Error:
       execution expired
     # ./spec/spec_helper.rb:47:in `sleep'
     # ./spec/spec_helper.rb:47:in `block (2 levels) in wait_for'
     # ./spec/spec_helper.rb:45:in `loop'
     # ./spec/spec_helper.rb:45:in `block in wait_for'
     # ./spec/spec_helper.rb:44:in `wait_for'
     # ./spec/lib/nsq/producer_spec.rb:230:in `block (3 levels) in <top (required)>'

Finished in 24.4 seconds (files took 0.222 seconds to load)
81 examples, 1 failure

Failed examples:

rspec ./spec/lib/nsq/producer_spec.rb:240 # Nsq::Producer connecting via nsqlookupd #connections should be connected to all nsqds

Do you have any plan of testing this on any CI service? I'll setup travis.yml on a specific branch so we can test different NSQ version at the same time, we just need to use specific docker images tag. Does it make sense for you?

freerobby commented 7 years ago

@pacoguzman Thanks for catching and working on this! I think we should maintain tests for old and new formats to be sure we don't unintentionally break on anyone. Then later on we can deprecate the old version and remove support in a major release when 1.0.0 is more ubiquitous in production.

Setting up Travis would be awesome!

pacoguzman commented 7 years ago

@freerobby I'll try to setup travis-ci (as we're open source) should be easy to get up an running the two versions using a travis matrix

bschwartz commented 7 years ago

@pacoguzman Mind bumping nsq-cluster to 2.1? I just updated it so it will support both NSQ < 1.0 and NSQ >= 1.0. With that change, tests pass for me locally with both NSQ 0.3.8 and NSQ 1.0.0.

Once that's done, this looks ready to merge! Thanks for your work here and for using nsq-ruby! Psyched to have Travis set up for this project too -- long overdue :)

pacoguzman commented 7 years ago

I've bumped the nsq-cluster dependency but I'm getting random timeout errors on Travis. Maybe we can review after merge it.

bschwartz commented 7 years ago

Thanks @pacoguzman!

pacoguzman commented 7 years ago

You're welcome!

On Mon, Apr 17, 2017, 10:22 PM Brendan Schwartz notifications@github.com wrote:

Thanks @pacoguzman https://github.com/pacoguzman!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wistia/nsq-ruby/pull/38#issuecomment-294580882, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAqrkhoYvhp55zPxWWw8AU275EH0KmJks5rw8oTgaJpZM4Mn9bX .

wistia / nsq-ruby

Make Nsq::Discovery compatible with v1.0.0-compat responses #38