Iterate on DLM Transition service#145081
Conversation
Add queuing to executor, improve tests and remove thread capabilities
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
📝 WalkthroughWalkthroughThis PR refactors the DLM frozen transition executor to implement bounded-queue task scheduling. It replaces set-based running-state tracking with a concurrent map that tracks both queued and active transitions by index name, adds a configurable Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
x-pack/plugin/dlm-frozen-transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutor.java (1)
50-56: Thread name customization won't work as intended.The
r instanceof DlmFrozenTransitionRunnablecheck will always be false—by the time the thread factory receivesr, it's wrapped in executor infrastructure (e.g.,FutureTask), not the original task.Not a merge blocker—functionality is correct, just thread names won't include the index.
Alternative: Pass index name via ThreadLocal or remove the dead code
If index-specific thread names are important, consider a
ThreadLocal<String>set inwrapRunnable()and read in the thread factory. Otherwise, this block can be removed since it never executes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/plugin/dlm-frozen-transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutor.java` around lines 50 - 56, The thread-name customization block inside the EsExecutors.newFixed factory doesn't work because the runnable handed to the thread factory is wrapped (e.g., FutureTask), so the `r instanceof DlmFrozenTransitionRunnable` check never matches; either remove that dead branch or implement the suggested ThreadLocal approach: set a ThreadLocal<String> (e.g., in DlmFrozenTransitionExecutor.wrapRunnable or where DlmFrozenTransitionRunnable is wrapped) with dftr.getIndexName() before wrapping and clear it after, then read that ThreadLocal in the thread factory used by EsExecutors.newFixed (EXECUTOR_NAME) to append the index to thread.getName(); reference DlmFrozenTransitionRunnable, wrapRunnable(), EsExecutors.newFixed/EXECUTOR_NAME and the thread factory to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@x-pack/plugin/dlm-frozen-transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutor.java`:
- Around line 50-56: The thread-name customization block inside the
EsExecutors.newFixed factory doesn't work because the runnable handed to the
thread factory is wrapped (e.g., FutureTask), so the `r instanceof
DlmFrozenTransitionRunnable` check never matches; either remove that dead branch
or implement the suggested ThreadLocal approach: set a ThreadLocal<String>
(e.g., in DlmFrozenTransitionExecutor.wrapRunnable or where
DlmFrozenTransitionRunnable is wrapped) with dftr.getIndexName() before wrapping
and clear it after, then read that ThreadLocal in the thread factory used by
EsExecutors.newFixed (EXECUTOR_NAME) to append the index to thread.getName();
reference DlmFrozenTransitionRunnable, wrapRunnable(),
EsExecutors.newFixed/EXECUTOR_NAME and the thread factory to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ef209135-e37f-45a3-b4ce-0ba669b69dcd
📒 Files selected for processing (6)
x-pack/plugin/dlm-frozen-transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutor.javax-pack/plugin/dlm-frozen-transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionPlugin.javax-pack/plugin/dlm-frozen-transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionService.javax-pack/plugin/dlm-frozen-transition/src/main/plugin-metadata/entitlement-policy.yamlx-pack/plugin/dlm-frozen-transition/src/test/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutorTests.javax-pack/plugin/dlm-frozen-transition/src/test/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionServiceTests.java
💤 Files with no reviewable changes (1)
- x-pack/plugin/dlm-frozen-transition/src/main/plugin-metadata/entitlement-policy.yaml
dakrone
left a comment
There was a problem hiding this comment.
LGTM, I left some minor comments, but nothing blocking. Thanks for working on this Luke!
...transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutor.java
Outdated
Show resolved
Hide resolved
...transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutor.java
Show resolved
Hide resolved
...transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutor.java
Outdated
Show resolved
Hide resolved
...-transition/src/main/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionService.java
Show resolved
Hide resolved
...ition/src/test/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutorTests.java
Outdated
Show resolved
Hide resolved
...ition/src/test/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionExecutorTests.java
Outdated
Show resolved
Hide resolved
...sition/src/test/java/org/elasticsearch/xpack/dlm/frozen/DlmFrozenTransitionServiceTests.java
Outdated
Show resolved
Hide resolved
* Iterate on DLM Transition service Add queuing to executor, improve tests and remove thread capabilities * Use named wrapper class for runnable to ensure thread name instanceof matches * CheckStyle fix * PR Changes
* Iterate on DLM Transition service Add queuing to executor, improve tests and remove thread capabilities * Use named wrapper class for runnable to ensure thread name instanceof matches * CheckStyle fix * PR Changes
This makes a few improvements to the DLM transition service: