Threadsafe Manticore

[andrewvc]

Continues work from #284 making Manticore connections threadsafe. Renamed the branch because this only touches Manticore.

[andrewvc]

@karmi can you give this a review?

[karmi]

@andrewvc, nice! I'm afraid I won't be able to do a full review this week, I need to keep focus on some internal applications, but I'll try to find some time, if I fail, let's sync in the beginning of the next week.

[karmi]

We are Ruby 1.8 compatible in the low-level client, what's the reason for removing 1.8.7 from the Travis configuration?

[andrewvc]

Hmm, I thought these were failing on Master. I see the unit tests are passing here: https://travis-ci.org/elastic/elasticsearch-ruby/builds/120013643 . I must have been mistaken

[karmi]

Some tests might have failed, but I'd like to keep 1.8 compatibility for the low level gems (transport+api) as long as technically possible. (The Rails integration and the elasticsearch-dsl gem are already 1.9+ only.)

[andrewvc]

Ruby 1.8.7-p370 was released Jun 29. 2012, almost four years ago at this point.

Are people still using it?

[jsvd]

You'd be amazed :D

[karmi]

I started leaving comments on specific points in the codebase, and will also leave general comments here.

The PR fails at TravisCI with An error occurred while installing manticore (0.5.5), and Bundler cannot continue -- do we know what's the reason for that?

[karmi]

I think that given how much logic is specific to the Manticore adapter, it would be safer to add real, integrations tests for it, loosely modeled after the client_test.rb?

[karmi]

What about with_enriched_response to better match with_request_retries?

[andrewvc]

Will make this not a block per @jsvd above.

[karmi]

Also, another "administrative" comment -- since the Manticore classes implement and add a suite of methods, can we have proper RDoc/Yardoc style documentation for them? Notably parameters and return values. (Many of those are "internal", so I don't think they need usage examples that much.)

[karmi]

Can we have a "header" documentation text here, describing the purpose of this class, and how it fits with the Manticore-specific architecture?

[jsvd]

s/to to/to

[jsvd]

I got the following test failure (seed 15751):

Elasticsearch::Transport::Transport::ManticoreSnifferTest
              PASS (0:00:09.134) test_: Manticore Sniffer should be initialized with a transport instance. 
I, [2016-04-11T11:20:32.797000 #28273]  INFO -- : Will sniff every 1 seconds for new Elasticsearch hosts!
              FAIL (0:00:14.154) test_: Manticore Sniffer should sniff every N seconds. 
          Expcted 5 or 4 invocations. Got 3
        @ test/unit/manticore_sniffer_test.rb:60:in `ManticoreSnifferTest'
          org/jruby/RubyBasicObject.java:1623:in `instance_exec'
          /Users/joaoduarte/.rvm/gems/jruby-1.7.23/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `create_test_from_should_hash'
          /Users/joaoduarte/.rvm/gems/jruby-1.7.23/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:401:in `create_test_from_should_hash'
          org/jruby/RubyArray.java:2414:in `map'
          org/jruby/RubyArray.java:2414:in `map'
          org/jruby/RubyArray.java:1613:in `each'

[jsvd]

see full run with the 2 errors here https://gist.github.com/jsvd/8e27df94a8448964f1ce11c20d87fb1b

[jsvd]

I'm no longer seeing test failures <3

[andrewvc]

@karmi I added the 1.8.7 tests back in, but they are not passing on master. https://github.com/elastic/elasticsearch-ruby/blob/master/elasticsearch/test/integration/client_integration_test.rb Uses 1.9 syntax.

FWIW I think it's fine to require people on old rubies to use old libraries.

[karmi]

That's an integration test and these are skipped on Ruby 1.8 intentionally -- see Travis configuration.

On 12. 4. 2016, at 21:18, Andrew Cholakian notifications@github.com wrote:

@karmi I added the 1.8.7 tests back in, but they are not passing on master. https://github.com/elastic/elasticsearch-ruby/blob/master/elasticsearch/test/integration/client_integration_test.rb Uses 1.9 syntax.

FWIW I think it's fine to require people on old rubies to use old libraries.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[jsvd]

@andrewvc I was able to make travis build successfully by applying the following patch:

diff --git a/.travis.yml b/.travis.yml
index 556d43b..1390ff7 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -36,7 +36,8 @@ before_script:
   - gem install bundler -v 1.11.2
   - rake setup
   - rake elasticsearch:update
-  - echo elasticsearch-* elasticsearch | xargs -n1 -IFF bash -c 'echo processing bundle for FF && cd FF && bundle && cd ..'
+  - find . -maxdepth 1 -type d -name 'elasticsearch*' | xargs -n1 -IFF bash -c 'echo "processing bundle for FF"; cd FF; BUNDLE_GEMFILE= bundle; cd ..'

 script:
   - SERVER=start TEST_CLUSTER_LOGS=/tmp/log TEST_BUILD_REF=tags/v$ES_VERSION TEST_CLUSTER_COMMAND=/tmp/elasticsearch-$ES_VERSION/bin/elasticsearch rake test:$TEST_SUITE
diff --git a/elasticsearch-extensions/elasticsearch-extensions.gemspec b/elasticsearch-extensions/elasticsearch-extensions.gemspec
index 2fcf967..c069ded 100644
--- a/elasticsearch-extensions/elasticsearch-extensions.gemspec
+++ b/elasticsearch-extensions/elasticsearch-extensions.gemspec
@@ -54,8 +54,4 @@ Gem::Specification.new do |s|
   if defined?(RUBY_VERSION) && RUBY_VERSION > '2.2'
     s.add_development_dependency "test-unit", '~> 2'
   end
-
-  # Gems for testing integrations
-  s.add_development_dependency "patron" unless defined?(JRUBY_VERSION) || defined?(Rubinius)
-  s.add_development_dependency "oj" unless defined?(JRUBY_VERSION) || defined?(Rubinius)
 end
diff --git a/elasticsearch-extensions/lib/elasticsearch/extensions/backup.rb b/elasticsearch-extensions/lib/elasticsearch/extensions/backup.rb
index 01a0a53..85eec0f 100644
--- a/elasticsearch-extensions/lib/elasticsearch/extensions/backup.rb
+++ b/elasticsearch-extensions/lib/elasticsearch/extensions/backup.rb
@@ -4,10 +4,8 @@ require 'pathname'
 require 'fileutils'

 require 'multi_json'
-require 'oj'

 require 'elasticsearch'
-require 'patron'

 module Backup
   module Database

[andrewvc]

We'll just merge this directly into the ls output. Too much fuss to put this into this lib.