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

Use POST to send bulk requests #1010

Closed
hakman opened this issue Dec 15, 2015 · 6 comments
Closed

Use POST to send bulk requests #1010

hakman opened this issue Dec 15, 2015 · 6 comments

Comments

@hakman
Copy link

hakman commented Dec 15, 2015

A RESTful design uses PUT for updates and POST for create. Elastica uses the bulk API to create new documents:
https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Bulk.php#L356

Would it be OK to change the way Elastica sends bulk requests from PUT to POST?
Elasticsearch docs also mention POST as the method for sending bulk requests, though the spec allows both:
http://www.elastic.co/guide/en/elasticsearch/reference/master/docs-bulk.html

@damienalexandre
Copy link
Contributor

Good point. No reason to use PUT here, but elasticsearch handle them exactly the same.

The official PHP client is also using POST.

@ewgRa
Copy link
Contributor

ewgRa commented Dec 16, 2015

I think you right and it must be synced and POST more right in this case, so +1 for POST.

@hakman Did you face with real problem when PUT used? Or there is no something behind?

@hakman
Copy link
Author

hakman commented Dec 16, 2015

We have a product that works as a proxy for Elasticsearch to accept and parse logs.
I am writing some examples for that using various languages and libs. One of them is Monolog and had trouble shipping logs with it, because of this issue.

We are also modifying our proxy, but thought this might help others in the future.

@otisg
Copy link

otisg commented Dec 16, 2015

+1 for supporting POST

@ruflin
Copy link
Owner

ruflin commented Dec 18, 2015

I remember there was a reason we used PUT, but I think it was related to one of the deprecated transports. So +1 on this change.

hakman pushed a commit to hakman/Elastica that referenced this issue Dec 18, 2015
@ruflin
Copy link
Owner

ruflin commented Dec 31, 2015

Closing as #1011 was merged.

@ruflin ruflin closed this as completed Dec 31, 2015
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

5 participants