search

karmi / retire

A rich Ruby API and DSL for the Elasticsearch search engine
http://karmi.github.com/retire/
MIT License
1.87k stars 533 forks source link

Should handle Resolv::ResolvError #748

Open threez opened 11 years ago

threez commented 11 years ago

If the name can't be resolved an Resolv::ResolvError might be thrown. This should also be handled by the automatic index creation.

karmi commented 11 years ago

Does this exception appear when using Faraday? Since Resolv is not loaded automatically by Ruby?

threez commented 11 years ago

No not really by faraday. But faraday supports many drivers. And currently Faraday only supports [:MissingDependency, :ClientError, :ConnectionFailed, :ResourceNotFound, :ParsingError, :TimeoutError] but what is, if one of the drivers throws this exception or something similar like Errno::EADDRNOTAVAIL? In our setup we use our own relover based on resolv (this is why i point this out). I guess the point is, that if you load the application the application should not fail because of a connection / name resolution can't be made. In that sense it is similar to #730.

karmi commented 11 years ago

I understand the picture, I'm just wary about rescuing generic Exception in the code in question... But if we want to 100% cover theses cases, there might not be any other way. What would you suggest?

threez commented 11 years ago

I guess i my specific case #730 would help. In other words turning off the automatic index creation. Otherwise maybe an public interface, where i can tell tire which errors should be handled additionally when using automatic index creation.

karmi commented 11 years ago

In other words turning off the automatic index creation.

But that is problematic in its own sense: you set up your index settings and mappings in Tire, and you expect them to be reflected. Otherwise dynamic mapping kicks in, and we're in lots of hard to explain/debug problems.

What would probably be the least offensive solution is to either provide some configuration option a la Tire.configure { model_auto_create_index false }, or extract the logic into a module which you choose not to include, a la the callbacks module...?

threez commented 11 years ago

A Tire.configure { model_auto_create_index false } is implementation of #730. This would help a lot already and would be great (i maybe over simplified the summary a bit). The question is just then if is helpful for users of tire if their app breaks to start if the index auto creation failed. I think this is a good default for development, but in some cases you might want to prevent this. So it might be, that people, that don't want do deactivate index creation are better of with Tire.configure { raise_if_model_auto_create_index_fails false }.

karmi commented 11 years ago

There's actually another discussion about this in #798, and I like the proposed solution of mapping(auto_create: false), since it's more fine-grained?

karmi commented 11 years ago

@threez Actually, if I understand your concern, the action.auto_create_index: false should be used in production, indeed. I'm leaning towards closing this issue in favour fo the suggestion of #798 by @brightbits. What do you think?

threez commented 11 years ago

I think this are two different issues. Sure they are related, but the error handling of problems during the creation is something different for me. Another example: Suppose the ElasticSearch instance is behind a reverse proxy (for https and auth) then the reverse proxy search might return a 504 if elastic search is unavailable. This would raise a Faraday::Error::ClientError that currently is not handled also. No connection and resolve issues are important but not the only problems.