apache / apache/druid

IncrementalIndex's `in` ThreadLocal doesn't need to be instance-level

Open
#8,886 1 comment 0 reactions 0 assignees View on GitHub
Contributions Welcome Improvement Starter
Dominant language
Java
Stars
14.1k
Forks
3.8k
Avg merge
2d 58m
Merged PRs (30d)
233

Description

`IncrementalIndex`'s `ThreadLocal in` can be `static final`. This `ThreadLocal` doesn't store anything *specific to a given `IncrementalIndex`*: rather, it's used to set a `Row` temporarily, and there is no situation when an `IncrementalIndex` could call recursively into another `IncrementalIndex`, so this temporary assignment cannot be disturbed.

As a safety net, we could add a check `if (in.get() != null) throw ISE(..); in.set(row);`

Furthermore, it would be ideal to get rid of `ThreadLocal` altogether (which would be beneficial for performance, because every `Aggregator` wouldn't need to query `ThreadLocal` upon every indexed row (`ThreadLocal.get()` is like `Map.get()`, not super cheap), and for reducing complexity, but this requires a significant refactoring of `Aggregator` interface and/or indexing logic, so saving this idea for a better time.

Contributor guide

Open the contributing guide

Research direction

Start by locating IncrementalIndex's instance-level ThreadLocal in the indexing path and inspect how Aggregator accesses it for each indexed row. Confirm that the temporary Row value is not shared across recursive IncrementalIndex calls, then make the ThreadLocal static final and add the proposed safety check if appropriate. The broader ThreadLocal removal is explicitly deferred.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
data, databases
Issue type
Refactor
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.