Fix compaction tasks reports getting overwritten by ac9817 · Pull Request #15981 · apache/druid

ac9817 · 2024-02-27T18:29:39Z

Description

A single compaction task could be splitted the into multiple index tasks based on the interval given in spec. In such cases, all the index tasks are run with same id and the task completion report is getting over written. This PR skips writing the report for each parallel index task and instead writes on the compaction task completion.

With this change instead of overwriting the reports file, we make multiple entries and it looks something like:

{
  "ingestionStatsAndErrors_2": {
    "type": "ingestionStatsAndErrors",
    "taskId": "compact_test_klkggepm_2024-02-28T17:27:17.327Z",
    "payload": {
      "ingestionState": "COMPLETED",
      "unparseableEvents": {
        "buildSegments": []
      },
      "rowStats": {
        "totals": {
          "buildSegments": {
            "processed": 3,
            "processedBytes": 1500,
            "processedWithError": 0,
            "thrownAway": 0,
            "unparseable": 0
          }
        }
      },
      "errorMsg": null,
      "segmentAvailabilityConfirmed": false,
      "segmentAvailabilityWaitTimeMs": 0,
      "recordsProcessed": {}
    }
  },
  "ingestionStatsAndErrors_1": {
    "type": "ingestionStatsAndErrors",
    "taskId": "compact_test_klkggepm_2024-02-28T17:27:17.327Z",
    "payload": {
      "ingestionState": "COMPLETED",
      "unparseableEvents": {
        "buildSegments": []
      },
      "rowStats": {
        "totals": {
          "buildSegments": {
            "processed": 3,
            "processedBytes": 1500,
            "processedWithError": 0,
            "thrownAway": 0,
            "unparseable": 0
          }
        }
      },
      "errorMsg": null,
      "segmentAvailabilityConfirmed": false,
      "segmentAvailabilityWaitTimeMs": 0,
      "recordsProcessed": {}
    }
  },
  "ingestionStatsAndErrors_0": {
    "type": "ingestionStatsAndErrors",
    "taskId": "compact_test_klkggepm_2024-02-28T17:27:17.327Z",
    "payload": {
      "ingestionState": "COMPLETED",
      "unparseableEvents": {
        "buildSegments": []
      },
      "rowStats": {
        "totals": {
          "buildSegments": {
            "processed": 3,
            "processedBytes": 1500,
            "processedWithError": 0,
            "thrownAway": 0,
            "unparseable": 0
          }
        }
      },
      "errorMsg": null,
      "segmentAvailabilityConfirmed": false,
      "segmentAvailabilityWaitTimeMs": 0,
      "recordsProcessed": {}
    }
  }
}

Key changed/added classes in this PR

CompactionTask
ParallelIndexSupervisorTask

This PR has:

georgew5656

this looks good to me, imo we don't need the task id in the keys of the reports though,

i think ingestionStatsAndErrors_0 ingestionStatsAndErrors_1 (similar to what is done by query_controller tasks) or maybe the interval that is being compacted instead would be good.

ac9817 · 2024-02-28T17:35:17Z

@georgew5656 Modified to ingestionStatsAndErrors_0, ingestionStatsAndErrors_1

arunramani · 2024-02-29T06:33:47Z

@cryptoe / @kfaraz this PR changes the structure of the task report for compaction. Are task reports considered part of a Druid API contract? In other words, are "breaking" changes okay for this?

gianm · 2024-03-01T16:19:39Z

@cryptoe / @kfaraz this PR changes the structure of the task report for compaction. Are task reports considered part of a Druid API contract? In other words, are "breaking" changes okay for this?

Is the current format documented? If so, we should consider options that preserve compatibility with previous documentation.

If not, changing the format is fair game. Although, if you are depending on the specific output format, you might want to add documentation, as otherwise it might be changed later in a way you don't expect.

suneet-s · 2024-03-01T17:32:46Z

I tested this change a little bit and found that if you run a compact task with no additional subtasks - the report is an empty json object. Before this change the report would have some details.

I think we should fix it so that the task report details are preserved for a compact task with no sub-tasks, even if we change the format of the report.

+1 for gian's suggestion of documenting the report so that other users don't break it once we've decided on a particular format. I think it would be a good idea to also add a test of some sort that validates the format of the report. There are a few integration tests for compaction - perhaps one of those would be a good place to add validation for the format of the task report.

suneet-s

+1 from me on the approach. Thanks for the integration test and docs @adithyachakilam!

kfaraz

Sorry, @adithyachakilam , I couldn't get to reviewing this PR sooner.

I have left some comments for better code readability. There is also a minor concern regarding holding too many reports and causing an OOM.
Since this PR has already been merged, the comments can be addressed in a follow-up PR.

kfaraz · 2024-03-05T04:03:57Z

      log.info("Generated [%d] compaction task specs", totalNumSpecs);

      int failCnt = 0;
+      Map<String, TaskReport> completionReports = new HashMap<>();


If the compaction is being run on several intervals (not very likely but still a possibility), can holding all the task reports in memory potentially cause an OOM exception? Currently, most of the task reports contain only ingestStatsAndErrors but they may contain other stuff in the future.

In the future, we should consider writing out the sub-reports in a streaming fashion alongwith the required changes to the TaskReportFileWriter API.
For now, we should add a guardrail here so that we don't try to hold too many reports in memory and fail with an OOM.

what do you think is a good size to hold then ?

ac9817 · 2024-03-05T06:02:37Z

@kfaraz Trying to address them here: #16042

…6042) * initial commit * comments * typo * comments * comments * remove var * initialize global var early * remove new line * small test fix * same fix another test

Fix compaction tasks reports geting overwrittened

6c8a7c5

github-actions Bot added the Area - Ingestion label Feb 27, 2024

only skip for compactiont task

f9a736a

ac9817 force-pushed the adithyachakilam/fix-compaction-task-reports-getting-overwritten branch from ed286b1 to f9a736a Compare February 27, 2024 19:05

georgew5656 reviewed Feb 27, 2024

View reviewed changes

address comments

68f4e19

ac9817 requested a review from georgew5656 February 28, 2024 17:36

fix boolean

a2d3e1c

georgew5656 reviewed Feb 29, 2024

View reviewed changes

Comment thread indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java

georgew5656 reviewed Feb 29, 2024

View reviewed changes

Comment thread ...n/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTask.java Outdated

georgew5656 reviewed Feb 29, 2024

View reviewed changes

Comment thread ...n/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTask.java

move boolean flag to task rather than spec

c176efd

ac9817 requested a review from georgew5656 February 29, 2024 19:46

rename variable

aa8c150

georgew5656 approved these changes Feb 29, 2024

View reviewed changes

add docs, fix missing case

3b1a166

github-actions Bot added the Area - Documentation label Mar 1, 2024

georgew5656 reviewed Mar 1, 2024

View reviewed changes

Comment thread docs/ingestion/tasks.md Outdated

ac9817 commented Mar 1, 2024

View reviewed changes

Comment thread docs/ingestion/tasks.md Outdated

Update docs/ingestion/tasks.md

f3a0262

georgew5656 self-requested a review March 1, 2024 20:52

Adithya Chakilam added 2 commits March 1, 2024 16:00

rename var

6de47f3

add task report decode check in IT

6d8fe75

suneet-s approved these changes Mar 2, 2024

View reviewed changes

change assert

4248561

georgew5656 merged commit ec52f68 into apache:master Mar 4, 2024

kfaraz reviewed Mar 5, 2024

View reviewed changes

This was referenced Mar 5, 2024

Enable segments read/published stats on Compaction task completion reports #15947

Merged

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

Merged

adarshsanjeev added this to the 30.0.0 milestone May 6, 2024

adarshsanjeev mentioned this pull request May 28, 2024

[DRAFT] 30.0.0 release notes #16505

Closed

Conversation

ac9817 commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Uh oh!

georgew5656 left a comment

Choose a reason for hiding this comment

Uh oh!

ac9817 commented Feb 28, 2024

Uh oh!

arunramani commented Feb 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gianm commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suneet-s commented Mar 1, 2024

Uh oh!

Uh oh!

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

kfaraz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfaraz Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

ac9817 Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ac9817 commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ac9817 commented Feb 27, 2024 •

edited

Loading

gianm commented Mar 1, 2024 •

edited

Loading

kfaraz left a comment •

edited

Loading