Fix Alias.all #679

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

Hi,
I open this PR to fix a check in alias.rb at line 97. This fix avoid raising of "NoMethodError: undefined method `empty?' for nil:NilClass" when the key 'aliases' is not present in value hash.

I add the fix because I am receiving that error when I call: Tire.index(index).aliases

NoMethodError: undefined method `empty?' for nil:NilClass

[GEM_ROOT]/gems/tire-0.5.5/lib/tire/alias.rb:97:in block in all' [GEM_ROOT]/gems/tire-0.5.5/lib/tire/alias.rb:95:ineach'
[GEM_ROOT]/gems/tire-0.5.5/lib/tire/alias.rb:95:in inject' [GEM_ROOT]/gems/tire-0.5.5/lib/tire/alias.rb:95:inall'
[GEM_ROOT]/gems/tire-0.5.5/lib/tire/index.rb:52:in `aliases'
...

Unfortunately I was unable to add a test to cover this behaviour: I tried to change index_alias_test and index_test without success.

Owner

karmi commented Mar 26, 2013

Hi, this is strange -- could you provide the exact request/responses from Elasticsearch (Tire.configure { logger STDERR, level: "debug" }) for the case where it fails?

Owner

karmi commented Mar 27, 2013

@stefanofontanelli Any new info about this? I'm interested in the specific case where this could happen...

I'm seeing the same thing when trying to populate a new Heroku Bonsai cluster. We have a rake task that populates a cluster using aliases, so that there is no downtime during reindexing. Our code may or may not assume there is an existing alias but we are noticing that on a local 0.20.6 elasticsearch AND on our self built multi-node ES cluster, this works fine. On the Bonsai cluster, it bails with:

NoMethodError: undefined method `empty?' for nil:NilClass
vendor/bundle/ruby/1.9.1/gems/tire-0.4.2/lib/tire/alias.rb:97:in `block in all'
vendor/bundle/ruby/1.9.1/gems/tire-0.4.2/lib/tire/alias.rb:95:in `each'
vendor/bundle/ruby/1.9.1/gems/tire-0.4.2/lib/tire/alias.rb:95:in `inject'
vendor/bundle/ruby/1.9.1/gems/tire-0.4.2/lib/tire/alias.rb:95:in `all'
vendor/bundle/ruby/1.9.1/gems/tire-0.4.2/lib/tire/alias.rb:117:in `find'

I'll email bonsai with this link to see if they have any input but I wanted to let you know this PR could be a general solution for people running into this kind of situation.

Owner

karmi commented Mar 29, 2013

@dfuentes77 Thanks for the info! I'm all for fixing this so people are sorted out, but would like to know the root cause for this behaviour.

@nz Nick, can you look into this? Why would Bonsai not return the standard ES response on /_aliases here?

Contributor

nz commented Mar 29, 2013

While nil checks are always a good idea, the root cause is ultimately my
issue to fix here. Basically the alias isn't being created properly. I can get
this sorted on our end.

Nick Zadrozny

Great. Let me know when I can test again on our enterprise addon. Thanks.

Owner

karmi commented Mar 29, 2013

@nz Thanks for confirmation. I'm not against being defensive in the code in question -- just that by default, Elasticsearch should always return valid JSON on myindex/_aliases.

Contributor

nz commented Mar 29, 2013

@karmi for sure, totally understood. We do some things in a slightly non-standard way out of necessity/opinionation. I'll break it down a bit further here, now that I'm back at a keyboard:

So we are returning JSON for that action — just with a different internal index name. So the JSON is kind of valid, in a boring technical sense, but mostly not, from the perspective of the user's intent.

For example, curl http://oak-12345.us-east-1.bonsai.io/acme-test/_aliases might return:

{"m3z6xmmzeja13q54ho9a":{"aliases":{}}}

Indexing the response by acme-test is what returns a nil here. This is similar to another ticket somewhere about fetching the index settings or mapping, or anything else where the index name is a key for the response.

My fix here will be part of a larger ongoing refactoring that does a more thorough response filtering, to map index names back to the user's specified logical name. Very slightly non-trivial to do when trying to preserve good performance, but we're working on it :)

Owner

karmi commented Mar 29, 2013

@nz Thanks for the explanation, should we close the ticket?

If this will take long, what can I do in the mean time? I can't populate our cluster with data because of this.

I am really sorry @karmi for my delay responding you.
I use ES on found.no, I use aliases in the same way reported by @dfuentes77 and I have same issues.
I am not able to reproduce because the errors is reported to me by airbrake many times but not always during uptime of the system. It is hard to me send you information about how to reproduce it because the architecture is pretty complex: I have a lot of concurrent processes that take messages from queues and query ES to perform their tasks.

I don't know the reason but I think that add a check for nil value should be done anyway :)
I will send you more information if I will able to find it.

Thank you for the great work and support.

Owner

karmi commented Mar 30, 2013

@stefanofontanelli No worries. The thing is, Elasticsearch returns a JSON response like this:

$ curl http://localhost:9200/_aliases?pretty
{
  "books" : {
    "aliases" : { }
  },
  "articles" : {
    "aliases" : {
      "myalias" : { }
    }
  }
}

$ curl http://localhost:9200/articles/_aliases?pretty
{
  "articles" : {
    "aliases" : {
      "myalias" : { }
    }
  }
}

So, when the code in question gets an incompatible JSON (MultiJson.decode('{"foo":"bar"}').inject({}) { |result, (index, value)| next result if value['aliases'].empty? ; result }). Now, as I said, I have nothing against defensive coding in principle, but:

a) this feels like a problem with ES providers such as Bonsai or Found, and they should return compatible JSON

b) in a dynamic system like this we would have to "double-check" every variable/Hash key/etc all the time, then a refactoring comes, nobody knows if the defensive code is safe to delete or not, since there's no test, ...

Again, as soon as there's a reliably failing test for the feature, I'm all for it :) Does it make sense?

Owner

karmi commented Mar 30, 2013

@nz Just notice that the Bonsai JSON output is perfectly valid for the code in question:

MultiJson.decode('{"m3z6xmmzeja13q54ho9a":{"aliases":{}}}').inject({}) { |result, (index, value)| next result if value['aliases'].empty? ; result }

It wouldn't probably work as expected, but AFAIK shouldn't throw the nil exception.

Also please see my comment above why "nil checks are always a good idea" is not neccessarily always the case :)

@karmi Yes it make sense :)
My Found.no cluster returns a JSON like this:

$ curl http://localhost:9200/_aliases?pretty
{
  "data-28-03-2013" : {
    "aliases" : {
      "data" : { }
    }
  },
  "data-30-03-2013" : {
    "aliases" : { }
  }
}

$ curl http://localhost:9200/data/_aliases?pretty
{
  "data-28-03-2013" : {
    "aliases" : {
      "data" : { }
    }
  }
}

$ curl http://localhost:9200/data-28-03-2013/_aliases?pretty
{
  "data-28-03-2013" : {
    "aliases" : {
      "data" : { }
    }
  }
}

$ curl http://localhost:9200/data-30-03-2013/_aliases?pretty
{
  "profiles-30-03-2013" : {
    "aliases" : { }
  }
}

$ curl http://localhost:9200/not-existent/_aliases?pretty
{}
Owner

karmi commented Mar 30, 2013

@stefanofontanelli All of them should pass the code: http://pastie.org/private/if88c0nll6gpkwpfkasyq

Closing this and let's reopen when we have more data/test/etc, OK?

karmi closed this Mar 30, 2013

@karmi Yes, make sense.

Contributor

nz commented Apr 2, 2013

Thanks all for the sleuthing, that was very educational for me as well.
Some further Alias related compatibility work still ongoing on my end for
Bonsai.

—Nick

Nick Zadrozny

Just so that everyone knows, I just tried using Found.no for the first time as well, and our job ran fine and populated the cluster with our data without error, so far. I'll be doing some more functional testing soon.

Contributor

nz commented Apr 10, 2013

Hi all — wrapping this up, Tire::Alias.all should now work as expected with Bonsai. Here's an excerpt from my testing:

ENV['ELASTICSEARCH_URL'] = 'http://user:pass@host.us-east-1.bonsai.io'
require 'tire'
# => true
Tire::Alias.all
# => [<Tire::Alias {:indices=><Tire::Alias::IndexCollection "test">, :name=>"aliastest"}>]
Tire::Alias.create(name: "testalias", indices: [ "test" ])
# => 200 : {"ok":true,"acknowledged":true}
Tire::Alias.all
# => [<Tire::Alias {:indices=><Tire::Alias::IndexCollection "test">, :name=>"aliastest"}>, <Tire::Alias {:indices=><Tire::Alias::IndexCollection "test">, :name=>"testalias"}>]
Tire::Alias.create(name: "testalias", indices: [ "notfound" ])
# => 404 : {"error":"Could not find index 'notfound'","status":404}
Tire::Alias.all
# => [<Tire::Alias {:indices=><Tire::Alias::IndexCollection "test">, :name=>"aliastest"}>, <Tire::Alias {:indices=><Tire::Alias::IndexCollection "test">, :name=>"testalias"}>]
Tire.index('test').aliases
# => [<Tire::Alias {:indices=><Tire::Alias::IndexCollection "80j6o802ev47bmkhmb2m">, :name=>"dswruzxh2fuhyageamth"}>, <Tire::Alias {:indices=><Tire::Alias::IndexCollection "80j6o802ev47bmkhmb2m">, :name=>"9s0md390kvfeyj4bglm5"}>]

As per the above, there is still some index name response mapping to be done when listing an index's aliases directly, but nothing should be breaking with nil errors. Do email me at support@bonsai.io if the internal index name thing is getting in your way.

Thanks. I'll test this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment