Prevent Duplicate ILM Cluster State Updates from Being Created #78390
Conversation
This file contains 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
WIP, deduplicating these tasks.
original-brownbear
added
WIP
:Data Management/ILM+SLM
original-brownbear
requested review from
martijnvg,
probakowski and
joegallo
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
probakowski
reviewed
Sep 28, 2021
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) { |
There was a problem hiding this comment.
any reason for explicit synchronized block and not using Collections.synchronizedSet(new HashSet<>()) or ConcurrentHashMap?
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 + "]"; |
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
|
Thanks @probakowski ! |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Sep 29, 2021
…ic#78390) Prevent duplicate ILM tasks from being enqueued to fix the most immediate issues around elastic#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 elastic#77466 Closes elastic#78246
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
to original-brownbear/elasticsearch
that referenced
this pull request
Sep 29, 2021
Follow up to elastic#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
to original-brownbear/elasticsearch
that referenced
this pull request
Sep 29, 2021
Follow up to elastic#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
to original-brownbear/elasticsearch
that referenced
this pull request
Sep 29, 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 elastic#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
original-brownbear
added a commit
to original-brownbear/elasticsearch
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 elastic#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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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