Prevent Duplicate ILM Cluster State Updates from Being Created#78390
Merged
original-brownbear merged 4 commits intoelastic:masterfrom Sep 29, 2021
original-brownbear:dedup-ilm
Merged
Prevent Duplicate ILM Cluster State Updates from Being Created#78390original-brownbear merged 4 commits intoelastic:masterfrom original-brownbear:dedup-ilm
original-brownbear merged 4 commits intoelastic:masterfrom
original-brownbear:dedup-ilm
Conversation
WIP, deduplicating these tasks.
Collaborator
|
Pinging @elastic/es-data-management (Team:Data Management) |
probakowski
reviewed
Sep 28, 2021
Contributor
probakowski
left a comment
There was a problem hiding this comment.
Thanks @original-brownbear, this change looks good. I left just one question and small suggestion.
I tested this locally and with creating 1000 indices at the same time and tasks count (_cat/pending_tasks) never went above 5000 comparing to 1000000 without this change.
| } | ||
|
|
||
| private boolean registerTask(IndexLifecycleClusterStateUpdateTask task) { | ||
| synchronized (executingTasks) { |
Contributor
There was a problem hiding this comment.
any reason for explicit synchronized block and not using Collections.synchronizedSet(new HashSet<>()) or ConcurrentHashMap?
Contributor
Author
There was a problem hiding this comment.
Right ... not sure why I did it this used the sync set now and just inlined everything since it's all one liners now :)
Thanks!
Comment on lines
+538
to
+542
| final boolean removed; | ||
| synchronized (executingTasks) { | ||
| removed = executingTasks.remove(task); | ||
| } | ||
| assert removed : "tried to unregister unknown task [" + task + "]"; |
Contributor
There was a problem hiding this comment.
Suggested change
| final boolean removed; | |
| synchronized (executingTasks) { | |
| removed = executingTasks.remove(task); | |
| } | |
| assert removed : "tried to unregister unknown task [" + task + "]"; | |
| synchronized (executingTasks) { | |
| final boolean removed = executingTasks.remove(task); | |
| assert removed : "tried to unregister unknown task [" + task + "]"; | |
| } |
probakowski
approved these changes
Sep 28, 2021
Contributor
probakowski
left a comment
There was a problem hiding this comment.
LGTM, thanks @original-brownbear!
Contributor
Author
|
Thanks @probakowski ! |
original-brownbear
added a commit
that referenced
this pull request
Sep 29, 2021
… (#78427) Prevent duplicate ILM tasks from being enqueued to fix the most immediate issues around #78246. The ILM logic should be further improved though. I did not include `MoveToErrorStepUpdateTask` in this change yet as I wasn't entirely sure how valid/safe hashing/comparing arbitrary `Exception`s would be. That could be looked into in a follow-up as well. Relates #77466 Closes #78246
original-brownbear
added a commit
that referenced
this pull request
Sep 29, 2021
Follow up to #78390. The `EmptyInfo` would not compare correctly because it doesn't implement equals or hashcode, breaking deduplication for `SetStepInfoUpdateTask`. => just making it a singleton to fix this and have a fast comp via instance equality.
original-brownbear
added a commit
that referenced
this pull request
Sep 29, 2021
Follow up to #78390. The `EmptyInfo` would not compare correctly because it doesn't implement equals or hashcode, breaking deduplication for `SetStepInfoUpdateTask`. => just making it a singleton to fix this and have a fast comp via instance equality.
original-brownbear
added a commit
that referenced
this pull request
Sep 30, 2021
If the current combination of current-step and index has a running CS update task enqueued there is no point in adding yet another task for this combination on the applier and we can skip the expensive inspection for the index. follow up to #78390
original-brownbear
added a commit
that referenced
this pull request
Sep 30, 2021
If the current combination of current-step and index has a running CS update task enqueued there is no point in adding yet another task for this combination on the applier and we can skip the expensive inspection for the index. follow up to #78390
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevent duplicate ILM tasks from being enqueued to fix the most immediate issues around #78246. The ILM logic should be further improved though. I did not include
MoveToErrorStepUpdateTaskin this change yet as I wasn't entirely sure how valid/safe hashing/comparing arbitraryExceptions would be. That could be looked into in a follow-up as well.Relates #77466
Closes #78246