-
Notifications
You must be signed in to change notification settings - Fork 831
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
Remove internal bulk processor retries #3739
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,8 +28,10 @@ import ( | |
"context" | ||
"errors" | ||
"fmt" | ||
"net/http" | ||
"time" | ||
|
||
"github.com/olivere/elastic/v7" | ||
enumspb "go.temporal.io/api/enums/v1" | ||
"go.temporal.io/sdk/temporal" | ||
"go.temporal.io/sdk/workflow" | ||
|
@@ -140,7 +142,8 @@ func (a *activities) AddESMappingFieldActivity(ctx context.Context, params Workf | |
_, err := a.esClient.PutMapping(ctx, params.IndexName, params.CustomAttributesToAdd) | ||
if err != nil { | ||
a.metricsHandler.Counter(metrics.AddSearchAttributesFailuresCount.GetMetricName()).Record(1) | ||
if esclient.IsRetryableError(err) { | ||
|
||
if a.isRetryableError(err) { | ||
a.logger.Error("Unable to update Elasticsearch mapping (retryable error).", tag.ESIndex(params.IndexName), tag.Error(err)) | ||
return fmt.Errorf("%w: %v", ErrUnableToUpdateESMapping, err) | ||
} | ||
|
@@ -152,6 +155,20 @@ func (a *activities) AddESMappingFieldActivity(ctx context.Context, params Workf | |
return nil | ||
} | ||
|
||
func (a *activities) isRetryableError(err error) bool { | ||
var esErr *elastic.Error | ||
if !errors.As(err, &esErr) { | ||
return true | ||
} | ||
|
||
switch esErr.Status { | ||
case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound, http.StatusConflict: | ||
return false | ||
default: | ||
return true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means we retry all non-ES errors. Is that what we want? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean "non-ES"? This error came from Elasticsearch and Elasticsearch uses http status codes to indicate error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I understand what you meant. Yes, non-ES errors are most likely some network failures and should be retryable. Also they might indicate some code bug (like bad formed url or missed required parameter). In this case it would be probably better not to retry but I don't know how to differentiate them. Generally, the idea is not to retry something that we know for sure is non-retryable and retry all the rest. |
||
} | ||
} | ||
|
||
func (a *activities) WaitForYellowStatusActivity(ctx context.Context, indexName string) error { | ||
if a.esClient == nil { | ||
a.logger.Info("Elasticsearch client is not configured. Skipping Elasticsearch status check.") | ||
|
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.
https://discuss.elastic.co/t/knowing-when-and-when-not-to-retry-a-request-based-on-elasticsearchexception-or-ioexception-with-the-resthighlevelclient/183779