-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Plumb DLM error store through to DlmFrozenTransition classes #145243
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| import org.elasticsearch.common.util.concurrent.EsExecutors; | ||
| import org.elasticsearch.common.util.concurrent.ThreadContext; | ||
| import org.elasticsearch.core.Strings; | ||
| import org.elasticsearch.dlm.DataStreamLifecycleErrorStore; | ||
| import org.elasticsearch.logging.Logger; | ||
| import org.elasticsearch.threadpool.ThreadPool; | ||
|
|
||
|
|
@@ -42,8 +43,9 @@ class DlmFrozenTransitionExecutor implements Closeable { | |
| private final ExecutorService executor; | ||
| private final int maxConcurrency; | ||
| private final int maxQueueSize; | ||
| private final DataStreamLifecycleErrorStore errorStore; | ||
|
|
||
| DlmFrozenTransitionExecutor(int maxConcurrency, int maxQueueSize, Settings settings) { | ||
| DlmFrozenTransitionExecutor(int maxConcurrency, int maxQueueSize, Settings settings, DataStreamLifecycleErrorStore errorStore) { | ||
| this.maxConcurrency = maxConcurrency; | ||
| this.maxQueueSize = maxQueueSize; | ||
| this.submittedTransitions = new ConcurrentHashMap<>(maxQueueSize); | ||
|
|
@@ -56,6 +58,7 @@ class DlmFrozenTransitionExecutor implements Closeable { | |
| } | ||
| return thread; | ||
| }, new ThreadContext(settings), EsExecutors.TaskTrackingConfig.DEFAULT); | ||
| this.errorStore = errorStore; | ||
| } | ||
|
|
||
| public boolean transitionSubmitted(String indexName) { | ||
|
|
@@ -112,7 +115,13 @@ public void run() { | |
| task.run(); | ||
| logger.debug("Transition completed for index [{}]", indexName); | ||
| } catch (Exception ex) { | ||
| logger.error(() -> Strings.format("Error executing transition for index [%s]", indexName), ex); | ||
| errorStore.recordAndLogError( | ||
| task.getProjectId(), | ||
| indexName, | ||
| ex, | ||
| Strings.format("Error executing transition for index [%s]", indexName), | ||
| 1 | ||
|
Contributor
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. Do we want all errors logged at WARN or can we go with the same value as DATA_STREAM_SIGNALLING_ERROR_RETRY_INTERVAL_SETTING and stick to every 10th?
Member
Author
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 was matching the behavior for now, I think we need to know whether we want this to be the one-stop-shop for error handling (in which case, we'll need to move that setting into core and add code to make it dynamic here) or whether we want to do more error handling in the converter. Let's discuss it. |
||
| ); | ||
| } finally { | ||
| submittedTransitions.remove(indexName); | ||
| } | ||
|
|
||
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.
Should these use the ES wrapper's instead?
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.
It would be nice if they did, but apparently the error store uses bits of the logger that our log manager does not support, so for now they have to remain using the log4j one.