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 elasticsearch create index race condition error #641

Merged

Conversation

pavolloffay
Copy link
Member

Fixes #589

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1f21f99 on pavolloffay:fix-es-index-race-condition into 813f04d on jaegertracing:master.

_, err := s.client.CreateIndex(indexName).Body(s.fixMapping(mapping)).Do(s.ctx)
s.writerMetrics.indexCreate.Emit(err, time.Since(start))
if err != nil {
return s.logError(jsonSpan, err, "Failed to create index", s.logger)
eErr, ok := err.(*elastic.Error)
if !ok || eErr.Details.Type != "index_already_exists_exception" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can Details be null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null check added

@coveralls
Copy link

coveralls commented Jan 11, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 3119246 on pavolloffay:fix-es-index-race-condition into 813f04d on jaegertracing:master.

@@ -140,12 +141,15 @@ func (s *SpanWriter) createIndex(indexName string, mapping string, jsonSpan *jMo
exists, _ := s.client.IndexExists(indexName).Do(s.ctx) // don't need to check the error because the exists variable will be false anyway if there is an error
if !exists {
// if there are multiple collectors writing to the same elasticsearch host, if the collectors pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole sentence is kinda wonky now.
"if there are multiple collectors attempting to create the same index at once, we make sure the error is not index_already_exists_exception or else we return the error"

or something to that effect

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b64071b on pavolloffay:fix-es-index-race-condition into ** on jaegertracing:master**.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling bedac86 on pavolloffay:fix-es-index-race-condition into 9f7228f on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5f0e3f6 on pavolloffay:fix-es-index-race-condition into 9f7228f on jaegertracing:master.

@pavolloffay pavolloffay merged commit 1988a90 into jaegertracing:master Jan 12, 2018
@ghost ghost removed the review label Jan 12, 2018
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.

4 participants