Skip to content

fixup!: #15981 Missing completion reports on index_parallel tasks#16042

Merged
georgew5656 merged 11 commits intoapache:masterfrom
ac9817:adithyachakilam/fix-15981
Mar 6, 2024
Merged

fixup!: #15981 Missing completion reports on index_parallel tasks#16042
georgew5656 merged 11 commits intoapache:masterfrom
ac9817:adithyachakilam/fix-15981

Conversation

@ac9817
Copy link
Copy Markdown
Contributor

@ac9817 ac9817 commented Mar 5, 2024

Description

Fixed the bug where completion task reports are not being generated on index_parallel tasks.

The fix made in #15981 misses to call the writeCompletionReports inside ParallelIndexSupervisorTask#runSequential.

Also, this PR tries to address the comments made on #15981 and #15930 after merged.


Key changed/added classes in this PR
  • CompactionTask
  • IndexTask
  • ParallelIndexSupervisorTask

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • 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
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@kfaraz
Copy link
Copy Markdown
Contributor

kfaraz commented Mar 5, 2024

Thanks for the quick fix, @adithyachakilam . I have left a few comments.

@ac9817 ac9817 force-pushed the adithyachakilam/fix-15981 branch from eb355d7 to dc0f4fb Compare March 5, 2024 16:08
@georgew5656
Copy link
Copy Markdown
Contributor

is this actually the behavior we want? it seems to contradict the behavior of other situations where we run a task inside another task where we expect the parent task to write its own reports

@georgew5656
Copy link
Copy Markdown
Contributor

is this actually the behavior we want? it seems to contradict the behavior of other situations where we run a task inside another task where we expect the parent task to write its own reports

looking at the code it seems to me all the other branches in ParallelIndexSupervisorTask call writeCompletionReports using the task report of the subtask generated (runRangePartitionMultiPhaseParallel, runSinglePhaseParallel, runHashPartitionMultiPhaseParallel)

I see there's already this code

    if (currentSubTaskHolder.setTask(sequentialIndexTask)
        && sequentialIndexTask.isReady(toolbox.getTaskActionClient())) {
      TaskStatus status = sequentialIndexTask.run(toolbox);
      completionReports = sequentialIndexTask.getCompletionReports();
      return status;
    } else {
      String msg = "Task was asked to stop. Finish as failed";
      LOG.info(msg);
      return TaskStatus.failure(getId(), msg);
    }

shouldn't we just call writeCompletionReports here instead?

@ac9817
Copy link
Copy Markdown
Contributor Author

ac9817 commented Mar 5, 2024

@georgew5656, That actually makes sense, fixed it!

@ac9817 ac9817 requested a review from georgew5656 March 5, 2024 20:08
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@georgew5656 georgew5656 merged commit ae022cc into apache:master Mar 6, 2024
@adarshsanjeev adarshsanjeev added this to the 30.0.0 milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants