148 lines
4.4 KiB
Markdown
148 lines
4.4 KiB
Markdown
# hive-0003: TaskTracker.updateTaskCount ArrayList visited O(T²) in REPL DAG traversal
|
||
|
||
## Classification
|
||
- **Severity**: MEDIUM
|
||
- **CWE**: CWE-407 (Algorithmic Complexity — Inefficient Algorithmic Complexity)
|
||
- **Component**: `ql/src/java/org/apache/hadoop/hive/ql/exec/repl/util/TaskTracker.java`
|
||
- **Methods**: `addTask()`, `addTaskList()`, `addDependentTask()`, `updateTaskCount()`
|
||
|
||
## Defect
|
||
|
||
`updateTaskCount()` traverses the full task DAG recursively to count tasks. The cycle-detection
|
||
accumulator `visited` is an `ArrayList<Task<?>>`, making each `visited.contains(childTask)` call
|
||
an O(T) linear scan. Because `updateTaskCount` is called once per task added — via `addTask`,
|
||
`addTaskList`, and `addDependentTask` — the total cost is O(T²) where T is the total number of
|
||
tasks (including transitive children) in the DAG.
|
||
|
||
In Hive replication load (`ReplLoadTask`), each bootstrap event may add dozens of tasks covering
|
||
tables, partitions, constraints, and functions. Large database replications with many tables
|
||
trigger this on every `addTask` call.
|
||
|
||
### Defective code (lines 65–101)
|
||
|
||
```java
|
||
// addTask
|
||
List <Task<?>> visited = new ArrayList<>();
|
||
updateTaskCount(task, visited);
|
||
|
||
// addTaskList
|
||
List <Task<?>> visited = new ArrayList<>();
|
||
for (Task<?> task : taskList) {
|
||
if (!visited.contains(task)) { // O(T) scan
|
||
tasks.add(task);
|
||
updateTaskCount(task, visited);
|
||
}
|
||
}
|
||
|
||
// updateTaskCount
|
||
private void updateTaskCount(Task<?> task, List <Task<?>> visited) {
|
||
numberOfTasks += 1;
|
||
visited.add(task);
|
||
if (task.getChildTasks() != null) {
|
||
for (Task<?> childTask : task.getChildTasks()) {
|
||
if (visited.contains(childTask)) { // O(T) scan — hot inner loop
|
||
continue;
|
||
}
|
||
updateTaskCount(childTask, visited);
|
||
}
|
||
}
|
||
}
|
||
```
|
||
|
||
## Fix
|
||
|
||
Change `List<Task<?>> visited` to `Set<Task<?>> visited` (use `new HashSet<>()`) in all three
|
||
call sites and in the signature of `updateTaskCount`. All `contains()` calls become O(1).
|
||
|
||
```java
|
||
// addTask
|
||
Set<Task<?>> visited = new HashSet<>();
|
||
updateTaskCount(task, visited);
|
||
|
||
// addTaskList
|
||
Set<Task<?>> visited = new HashSet<>();
|
||
for (Task<?> task : taskList) {
|
||
if (!visited.contains(task)) { // O(1)
|
||
tasks.add(task);
|
||
updateTaskCount(task, visited);
|
||
}
|
||
}
|
||
|
||
// updateTaskCount
|
||
private void updateTaskCount(Task<?> task, Set<Task<?>> visited) {
|
||
numberOfTasks += 1;
|
||
visited.add(task);
|
||
if (task.getChildTasks() != null) {
|
||
for (Task<?> childTask : task.getChildTasks()) {
|
||
if (visited.contains(childTask)) { // O(1)
|
||
continue;
|
||
}
|
||
updateTaskCount(childTask, visited);
|
||
}
|
||
}
|
||
}
|
||
```
|
||
|
||
## Complexity
|
||
|
||
| Scenario | Before | After |
|
||
|----------|--------|-------|
|
||
| T tasks total | O(T²) | O(T) |
|
||
| 100 tasks | 10,000 ops | 100 ops |
|
||
| 1000 tasks (large DB repl) | 1,000,000 ops | 1,000 ops |
|
||
|
||
**Speedup**: 100×–1000× for large REPL loads.
|
||
|
||
## Patch
|
||
|
||
```diff
|
||
--- a/ql/src/java/org/apache/hadoop/hive/ql/exec/repl/util/TaskTracker.java
|
||
+++ b/ql/src/java/org/apache/hadoop/hive/ql/exec/repl/util/TaskTracker.java
|
||
@@ imports @@
|
||
+import java.util.HashSet;
|
||
+import java.util.Set;
|
||
|
||
public void addTask(Task<?> task) {
|
||
tasks.add(task);
|
||
- List <Task<?>> visited = new ArrayList<>();
|
||
+ Set<Task<?>> visited = new HashSet<>();
|
||
updateTaskCount(task, visited);
|
||
}
|
||
|
||
public void addTaskList(List <Task<?>> taskList) {
|
||
- List <Task<?>> visited = new ArrayList<>();
|
||
+ Set<Task<?>> visited = new HashSet<>();
|
||
for (Task<?> task : taskList) {
|
||
if (!visited.contains(task)) {
|
||
tasks.add(task);
|
||
updateTaskCount(task, visited);
|
||
}
|
||
}
|
||
}
|
||
|
||
public void addDependentTask(Task<?> dependent) {
|
||
if (tasks.isEmpty()) {
|
||
addTask(dependent);
|
||
} else {
|
||
DAGTraversal.traverse(tasks, new AddDependencyToLeaves(dependent));
|
||
- List<Task<?>> visited = new ArrayList<>();
|
||
+ Set<Task<?>> visited = new HashSet<>();
|
||
updateTaskCount(dependent, visited);
|
||
}
|
||
}
|
||
|
||
private void updateTaskCount(Task<?> task,
|
||
- List <Task<?>> visited) {
|
||
+ Set<Task<?>> visited) {
|
||
numberOfTasks += 1;
|
||
visited.add(task);
|
||
if (task.getChildTasks() != null) {
|
||
for (Task<?> childTask : task.getChildTasks()) {
|
||
if (visited.contains(childTask)) {
|
||
continue;
|
||
}
|
||
updateTaskCount(childTask, visited);
|
||
}
|
||
}
|
||
}
|
||
```
|