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

Fix for bulk indexing error (#20) plus TTL addition #21

Merged
merged 8 commits into from
Jul 12, 2013
Merged

Fix for bulk indexing error (#20) plus TTL addition #21

merged 8 commits into from
Jul 12, 2013

Conversation

araddon
Copy link
Contributor

@araddon araddon commented Jul 5, 2013

This has unit tests and addresses #20.

Also includes:

  • a (unfortunate) breaking api change for adding ttl parameter to bulk indexing.
  • exposing the GlobalBulkIndexor

@mattbaird
Copy link
Owner

Hey Aaron, good change. I don't know what the etiquette is for breaking changes. Should we put something on the readme?

@araddon
Copy link
Contributor Author

araddon commented Jul 10, 2013

Yes, i feel really bad about the breaking change but didn't really see a way around it. Yes, Let me add something to the Readme. Also, maybe a note about the Search DSL api being still open to changes? I continue to find new features in ES search dsl which to meet require changes.

@mattbaird
Copy link
Owner

I like that plan. Better to break it early. Thanks. I will merge when you
submit a pr with the updated readme.
On Jul 10, 2013 9:19 AM, "Aaron Raddon" notifications@github.com wrote:

Yes, i feel really bad about the breaking change but didn't really see a
way around it. Yes, Let me add something to the Readme. Also, maybe a note
about the Search DSL api being still open to changes? I continue to find
new features in ES search dsl which to meet require changes.


Reply to this email directly or view it on GitHubhttps://github.com//pull/21#issuecomment-20753626
.

@dmichael
Copy link
Contributor

@araddon - out of curiosity (and not fully reading the code yet), is it possible to get the responses from the bulk indexer as well? - specifically concerning responses where there errors or for checking for duplicates. Looking forward to this merge - thanks for this work.

@araddon
Copy link
Contributor Author

araddon commented Jul 11, 2013

@dmichael , I updated the example doc to show usage to inspect the response bytes. However, @mattbaird the http.Response isn't available in the underlying api.DoCommand. What would you think about creating a new api.DoWithResponse?

Also, here is how you would get the http response until that change is made:

indexor := core.NewBulkIndexor(10)
// Create a custom Sendor Func, to allow inspection of response/error
indexor.BulkSendor = func(buf *bytes.Buffer) error {
    req, err := api.ElasticSearchRequest("POST", "/_bulk")
    if err != nil {
        return err
    }
    req.SetBody(buf)
    res, err := http.DefaultClient.Do((*http.Request)(req))
    if err != nil {
        return err
    }
    return err
}
done := make(chan bool)
indexor.Run(done)

@dmichael
Copy link
Contributor

Awesome, thanks for the examples - so whattya think @mattbaird, can we get a merge? The current implementation of bulk in master is not completely functional/robust ... this looks to be a big improvement

@mattbaird
Copy link
Owner

will do.

On Thu, Jul 11, 2013 at 9:51 AM, David Michael notifications@github.comwrote:

Awesome, thanks for the examples - so whattya think @mattbairdhttps://github.com/mattbaird,
can we get a merge? The current implementation of bulk in master is not
completely functional/robust ...


Reply to this email directly or view it on GitHubhttps://github.com//pull/21#issuecomment-20825765
.

mattbaird added a commit that referenced this pull request Jul 12, 2013
Fix for bulk indexing error (#20) plus TTL addition
@mattbaird mattbaird merged commit 42a237e into mattbaird:master Jul 12, 2013
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

Successfully merging this pull request may close these issues.

None yet

3 participants