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

Allow building client by specifying URL #120

Closed
plaflamme opened this issue Jan 3, 2012 · 13 comments
Closed

Allow building client by specifying URL #120

plaflamme opened this issue Jan 3, 2012 · 13 comments

Comments

@plaflamme
Copy link

Some elasticsearch servers may not be rooted on /.

It would be useful in these cases to allow building a client with a URL instead of a host/port combination:

$elascticaClient = new Elastica_Client(array(
        'url' => 'http://mydomain.org:7865/somePath/'
));
@ruflin
Copy link
Owner

ruflin commented Jan 3, 2012

Interesting idea. Is it possible to configure elasticsearch like this or are you using a proxy or something similar?

I see two options:

  • url: like you propose it
  • path: A third path variable that is added in front to every request

In a way I think best would be to implement both. If url is set, all the others are overwritten, if not, the option set or the default is set. Both should also be possible in the server array for multiple server. What do you think?

@plaflamme
Copy link
Author

It's probably a common use-case to proxy Elasticsearch HTTP, either for load-balancing or authorization concerns.

I think your approach is sensible (url and path). Both would cover this use-case.

@ruflin
Copy link
Owner

ruflin commented Jan 8, 2012

I added some code the to the master repository. Now it should be possible to set path or url. As I don't have a proxy setup here, can you try this out?

@plaflamme
Copy link
Author

Yep, it works. It requires that the url end with a '/' though. Maybe you could make it friendlier by detecting whether the provided url ends with a '/' and append one when it doesn't.

@ruflin
Copy link
Owner

ruflin commented Jan 10, 2012

Ok, good two know. So there are two options:

  • write a good documentation
  • or we add it automatically

It looks like you prefer point 2. The issue I see here, that perhaps we need it one day with /, and than we have some backward compatiblity.

One more suggestion: if / is missing, we throw an exception. Instead of "fixing" the code we point out the errors from the beginning. What do you think?

@plaflamme
Copy link
Author

Well, I very much doubt that someone would mount ES under something else than a 'directory'. It would result in URLs like so:

http://eshost/es_search
http://eshost/esmy_index/type/2
http://eshost/es_cluster/state
etc.

If there's someone out there that does this, then he or she is alone (doing this is simply looking for trouble). I don't see much benefit in supporting a single person's needs. I'd still vote for adding it automatically to help with the 99% use case.

My 2 cents...

@ruflin
Copy link
Owner

ruflin commented Jan 10, 2012

I just don't like that I have to fix in my code the code from someone else ;-) Do you think throwing exception is a good idea?

@plaflamme
Copy link
Author

sure, but doing so will simply push the problem down to your users. Every calling code will have to test the url parameter before passing it in to your library... same thing, different place.

@ruflin
Copy link
Owner

ruflin commented Jan 11, 2012

Sorry to go on with the arguing ;-) I'm not sure with we talk about the same use case. My assumption is: The path is set in a config file or hardcoded into the code. So it is nothing dynamic that changes too often, right? So the developer (not the user) has to add the path. Or how does your usecase look like?

@plaflamme
Copy link
Author

It's true that the URL may be configured by the actual user of Elastica, but it might not. For example, the Drupal module for using Elasticsearch with SearchAPI has a form where the Drupal end-user types the URL. In this case, the Drupal module would have to validate the URL... no big deal, really...

@ruflin
Copy link
Owner

ruflin commented Jan 31, 2012

I probably go with the option, that the Drupal module has to fix this issue ;-) I hope this is ok for you.

@lavoiesl
Copy link
Contributor

Can this issue be closed then ?

@ruflin
Copy link
Owner

ruflin commented Jul 26, 2012

Closed

@ruflin ruflin closed this as completed Jul 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants