apache / apache/druid

Get rid of instance-level ThreadLocal in ConcurrentGrouper

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

Description

Motivation: instance-level ThreadLocals are hard to understand. They are supposed to mask an issue of overly complex logic organization, but actually only add more complexity.

Instance-level ThreadLocals also create extra work for GC (weak references, large ThreadLocalMaps to scan) that can lead to longer pauses, but this is probably not a big issue in this specific case.

In the case of `ConcurrentGrouper`, `threadNumber` assignment could be pulled up to the beginning of the [task in `GroupByMergingQueryRunnerV2`](https://github.com/apache/incubator-druid/blob/17d773dca2e10529912d0e645170fe0eb5a13d35/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunnerV2.java#L232-L235), then a special accumulator is created which knows of the number (instead of using a pre-created accumulator from `RowBasedGrouperHelper.createGrouperAccumulatorPair()`), and that accumulator calls to a specially created `ConcurrentGrouper`'s method `aggregate(int threadNumber, key)`.

This means that creating concurrent and non-concurrent groupers should be un-generalized (currently generalized in `RowBasedGrouperHelper.createGrouperAccumulatorPair()` method), but this seems to be beneficial because this looks like a false abstraction which only adds coupling and confusion of execution paths (harder to see which grouper type is created and used when) rather than lifts any logic. For that matter, `ConcurrentGrouper` doesn't need to implement `Grouper` interface, too, providing *only* `aggregate(int threadNumber, key)` method, but not standard `Grouper`'s methods `aggregate(key)` and `aggregate(key, keyHash)`. I think it would also make things clearer because it would become more evident for people who try to unwind the logic of the subsystem in their heads where `ConcurrentGrouper` could and could not appear.

It would also be nice to provide evidence in comments that the number of queryables in `GroupByMergingQueryRunnerV2` and therefore the number of created tasks doesn' exceed (or exactly match?) the `concurrencyHint` because currently, this looks to be the prerequisite for `ConcurrentGrouper` to work without ArrayIndexOutOfBoundsException [in this line](https://github.com/apache/incubator-druid/blob/17d773dca2e10529912d0e645170fe0eb5a13d35/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java#L171), but it doesn't seems to follow from anywhere. Also, it means that `concurrencyHint` is not actually a "hint", that is, something completely heuristical/performance-related which could be any number, but actually a parameter which should have a very specific meaning of the actual number of concurrent threads calling `aggregate()` on the instance of `ConcurrentGrouper`.

FYI @jihoonson

Contributor guide

Open the contributing guide

Research direction

Start with ConcurrentGrouper.java, GroupByMergingQueryRunnerV2.java, and RowBasedGrouperHelper.createGrouperAccumulatorPair(). Trace how threadNumber and accumulators flow through the GroupByMergingQueryRunnerV2 task, then verify the concurrencyHint relationship described in the issue. Done means the instance-level ThreadLocal and unnecessary abstraction are removed without losing correct concurrent grouping behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
32/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.