kfaraz opened a new pull request, #14293:
URL: https://github.com/apache/druid/pull/14293
### Description
The `TaskQueue` in the Overlord implements concurrency control using a
`giant` lock. A similar technique has been used in other classes such as
`TaskMaster` and `TaskLockbox`. While this `giant` lock does guarantee
thread-safe access of critical sections of code, it can be too restrictive at
times and can even lead to the Overlord being completely stuck.
A typical scenario is described below.
- Insertion of a sub-task of an `index_parallel` task fails with a
`SQLTransientException` (say, due to an oversized payload)
- The `index_parallel` repeatedly requests the Overlord to insert this
sub-task
- Each time, the Overlord tries to insert this task up to 10 times (using
`RetryUtils`)
- While the Overlord is trying to insert, the calling thread holds the
`TaskQueue.giant` lock
- This causes the Overlord to essentially hang as no other `TaskQueue`
operation can proceed without the lock. This includes operations like adding a
new task, killing a task, submitting tasks to runner for execution, syncing
from metadata, etc.
Note: The indefinite retry issue in the above scenario is also being
addressed separately in #14271
### Current implementation
The `giant` lock is a reentrant lock, which is effectively the same as the
object monitor associated with any java object. In principle, this lock could
be replaced by simply making all the methods of `TaskQueue` `synchronized`.
The following operations/fields are protected by this lock.
- Methods: `start()`, `stop()`, `syncFromStorage()`, `manage()`
- Field `tasks`: `putIfAbsent()`, `get()`, `values()`, `remove()`
- Field `taskFutures`: `put()`, `remove()`
- Field `taskStorage`: `insert()`
- Field `taskLockbox`: `add()`, `remove()`
### Proposed implementation
| Method/Field | Change | Rationale |
|--------------|---------|-----------|
| Methods:<br>`start()`<br>`stop()`<br>`syncFromStorage()`<br>`manage()` |
Make methods `synchronized` | This effectively remains the same as the current
implementation |
| Field: `tasks`<br>Type: `LinkedHashMap<String, Task>`<br>Methods:
`putIfAbsent()`, `get()`, `remove()` | Use a `ConcurrentHashMap` instead in
conjunction with a `BlockingDeque<String>` to maintain order | The only
concurrency control needed here is at a task level, which can be easily ensured
by a `ConcurrentHashMap`. The `BlockingDeque` is used to maintain the order in
which task IDs were submitted to the `TaskQueue`. The updates to these data
structures are made atomically using `compute()` and `computeIfAbsent()`. |
| Field: `taskFutures`<br>Type: `HashMap<String, Future>` | Replace with
`ConcurrentHashSet<String> submittedTaskIds` | This is just needed to maintain
a set of task ids that have already been submitted to the `TaskRunner` |
| Field: `TaskStorage`<br>Methods: `insert()` | Move outside critical
section | `TaskStorage` implementations do not maintain any state and thus
don't require any concurrency control |
| Field: `TaskLockbox`<br>Methods: `add()`, `remove()` | Move outside
critical section | `TaskLockbox` has its own `giant` lock and can thus be
safely accessed here. |
### Other changes
-
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency
checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md)
(Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked
related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in
[licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [x] added comments explaining the "why" and the intent of the code
wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths,
ensuring the threshold for [code
coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md)
is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
