-
Notifications
You must be signed in to change notification settings - Fork 41
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
Support for ES 5.5 API and other fixes #9
Conversation
Fixed path used to Bulk index on 5.5 Added content-type header (required on 5.5) Fixed wrong BulkResponse.Item struct Added better error information iterating over BulkResponse.Item data. Changed JSON package to jsoniter (https://github.com/json-iterator/go) much faster than native encoding/json
Added content-type header (required on 5.5 API)
This fixed the pull request #7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR. Overall this looks great! One issue I have is jsoniter - which in my experience behaves differently from encoding/json in some cases. Please revert back to encoding/json.
indexing.go
Outdated
@@ -110,7 +115,8 @@ func BulkIndex(docs []string, options Options) error { | |||
if len(docs) == 0 { | |||
return nil | |||
} | |||
link := fmt.Sprintf("%s://%s:%d/%s/%s/_bulk", options.Scheme, options.Host, options.Port, options.Index, options.DocType) | |||
link := fmt.Sprintf("%s://%s:%d/_bulk", options.Scheme, options.Host, options.Port) | |||
// link := fmt.Sprintf("%s://%s:%d/%s/%s/_bulk", options.Scheme, options.Host, options.Port, options.Index, options.DocType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, this reduces repetition. Please just remove the commented out line.
indexing.go
Outdated
@@ -123,7 +129,7 @@ func BulkIndex(docs []string, options Options) error { | |||
// use it in the header. | |||
if options.IDField != "" { | |||
var docmap map[string]interface{} | |||
dec := json.NewDecoder(strings.NewReader(doc)) | |||
dec := jsoniter.NewDecoder(strings.NewReader(doc)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried jsoniter for another projects and switched back to encoding/json, since jsoniter could not handle cases encoding/json could handle just fine. Unfortunately, I haven't filed a bug report against jsoniter, I should do this some times, since it's a good project otherwise. The json encoding might also not be the top bottleneck at the moment.
So please revert jsoniter to encoding/json for the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand and agree, I had some issues with jsoniter too, but I filed an issue that has fixed over nigth.
Any way I'll revert to native json
Deleted useless commented line
Wonderful, thanks. Maybe we can bring in jsoniter later. |
Released 0.4.4 linux packages. Thanks again! |
Hi,
I made some changes on code, in order to support current Elastic Search (5.5) API.
Other changes on error information (Item struct)
Replace the JSON package by JSONITER, for improved performance.