Fix ingestion task failure when no input split to process by maytasm · Pull Request #11553 · apache/druid

maytasm · 2021-08-05T09:27:28Z

Fix ingestion task failure when no input split to process

Description

This PR fix a regression introduced by #11189
The regression causes ingestion task in parallel mode to fail when there is no input split to process (previously the task would succeed).
This is because of a null pointer exception occurred in getReports() at

druid/indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexPhaseRunner.java

Line 296 in 2df4214

public Map<String, SubTaskReportType> getReports()

. This is because this method accesses taskMonitor without checking if it is null or not. This is in contrast to all other code in the class, which check if taskMonitor is null or not. Also see in the run() method of the class

druid/indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexPhaseRunner.java

Line 111 in 2df4214

public TaskState run() throws Exception

,
that taskMonitor is initialized in line 121, but that does not always happen. If the method returned in line 116, which is when there is no input source to split, then taskMonitor stays null.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
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.

suneet-s · 2021-08-05T13:41:16Z

This sounds like an interesting edge case. ~~Hope can there be an interesting y'all with no input split?~~ How can there be an ingestion spec with no input split

I think we should add an integration test for this.

EDIT: Wow auto-correct on my phone - fixed my comment

jihoonson

Good catch @maytasm. Thanks for fixing this bug.

jihoonson · 2021-08-05T16:19:16Z

+    if (taskMonitor != null) {
+      taskMonitor.collectReport(report);
+    }


collectReport should be called only when there is a subtask sending its report. Since TaskMonitor is responsible for spawning subtasks, it seems quite strange if taskMonitor is null when this method is called. How about adding a precondition that explodes when it's null?

I just saw that the taskMonitor was accessed without a null check so I added the null check just in case. The bug mentioned in this ticket can be fix with just the change in getReports(). Given the context from your comment, maybe it is better to leave it as is and adds a comment mentioning something like... collectReport is only called when there is a subtask sending its report. Since TaskMonitor is responsible for spawning subtasks, the taskMonitor cannot be null

The comment sounds good. I think having a null check is also a good convention because someone like me could break the contract around taskMonitor unintentionally in the future. Perhaps we can add assert taskMonitor != null;. This will be ran only in tests, but not in production.

Is there a way to have static analysis flag all these uses? I've tried using combinations of @Nullable(doesn't realize that once it's set, it's good for the rest of the method) and @MonotonicNonNull but haven't been able to get intelliJ to flag these issues for me locally yet. I figured I would just leave a comment here and maybe you will have better luck

You can set the severity of the "Nullability problems" intellij inspection to error instead of warning. I think we haven't done it because there could be lots of places where the inspection will complain about nulls. It could be worth doing it at some point though.

jihoonson · 2021-08-05T16:19:24Z

  public Map<String, SubTaskReportType> getReports()
  {
-    return taskMonitor.getReports();
+    return taskMonitor == null ? Collections.emptyMap() : taskMonitor.getReports();


maytasm · 2021-08-07T07:32:34Z

This sounds like an interesting edge case. ~~Hope can there be an interesting y'all with no input split?~~ How can there be an ingestion spec with no input split

I think we should add an integration test for this.

EDIT: Wow auto-correct on my phone - fixed my comment

To name a few cases:

Reindex with datasource name that doesn’t exist
Reindex with interval that does not contain any data
Ingestion with interval in granularitySpec mismatching the inputSpec

maytasm · 2021-08-07T07:49:08Z

This sounds like an interesting edge case. ~~Hope can there be an interesting y'all with no input split?~~ How can there be an ingestion spec with no input split

I think we should add an integration test for this.

EDIT: Wow auto-correct on my phone - fixed my comment

Added integration test

suneet-s

LGTM - Thanks for the integration test!

suneet-s · 2021-08-09T14:35:46Z

To name a few cases:

Reindex with datasource name that doesn’t exist

Reindex with interval that does not contain any data

Ingestion with interval in granularitySpec mismatching the inputSpec

The first 2 cases sound like a user doing something they shouldn't be doing - and the re-index is a no-op. Maybe we should surface this as a typo to the end user?

Can you provide an example of the 3rd case?

I'm wondering if all these cases are situations where the end user should be warned of something "unexpected" happening.

maytasm · 2021-08-09T14:52:01Z

To name a few cases:

Reindex with datasource name that doesn’t exist

Reindex with interval that does not contain any data

Ingestion with interval in granularitySpec mismatching the inputSpec

The first 2 cases sound like a user doing something they shouldn't be doing - and the re-index is a no-op. Maybe we should surface this as a typo to the end user?

Can you provide an example of the 3rd case?

I'm wondering if all these cases are situations where the end user should be warned of something "unexpected" happening.

For the 3rd case, imagine you have a hash partitioning with the following spec:

   ...
    "ioConfig": {
      "type": "index_parallel",
      "inputSource": {
        "type": "druid",
        "dataSource": "mydatasource",
        "interval": "2020-07-31T00:00:00.000Z/2020-08-01T00:00:00.000Z",
        "filter": null,
        "dimensions": null,
        "metrics": [
          ...
        ]
      },
      "inputFormat": null,
      "appendToExisting": false
    },
    ....
    "granularitySpec": {
        "type": "uniform",
        "segmentGranularity": "DAY",
        "queryGranularity": "HOUR",
        "rollup": true,
        "intervals": [
          "2020-08-01T00:00:00.000Z/2020-08-02T00:00:00.000Z"
        ]
      },
    ....
    "partitionsSpec": {
        "type": "hashed",
        "numShards": null,
        "partitionDimensions": [],
        "partitionFunction": "murmur3_32_abs",
        "maxRowsPerSegment": 5000000
      },
    ....

The partial_segment_merge phase tasks will have no input split to process as the interval in the ioConfig does not match with intervals in granularitySpec

maytasm · 2021-08-09T14:53:55Z

To name a few cases:

Reindex with datasource name that doesn’t exist

Reindex with interval that does not contain any data

Ingestion with interval in granularitySpec mismatching the inputSpec

The first 2 cases sound like a user doing something they shouldn't be doing - and the re-index is a no-op. Maybe we should surface this as a typo to the end user?
Can you provide an example of the 3rd case?
I'm wondering if all these cases are situations where the end user should be warned of something "unexpected" happening.

For the 3rd case, imagine you have a hash partitioning with the following spec:
   ...
    "ioConfig": {
      "type": "index_parallel",
      "inputSource": {
        "type": "druid",
        "dataSource": "mydatasource",
        "interval": "2020-07-31T00:00:00.000Z/2020-08-01T00:00:00.000Z",
        "filter": null,
        "dimensions": null,
        "metrics": [
          ...
        ]
      },
      "inputFormat": null,
      "appendToExisting": false
    },
    ....
    "granularitySpec": {
        "type": "uniform",
        "segmentGranularity": "DAY",
        "queryGranularity": "HOUR",
        "rollup": true,
        "intervals": [
          "2020-08-01T00:00:00.000Z/2020-08-02T00:00:00.000Z"
        ]
      },
    ....
    "partitionsSpec": {
        "type": "hashed",
        "numShards": null,
        "partitionDimensions": [],
        "partitionFunction": "murmur3_32_abs",
        "maxRowsPerSegment": 5000000
      },
    ....
    
The partial_segment_merge phase tasks will have no input split to process as the interval in the ioConfig does not match with intervals in granularitySpec

The cases I thought of are due to user error in their ingestionSpec. I am not sure if there are valid cases where we can run into no input split to process situation though.

jihoonson

LGTM

I'm wondering if all these cases are situations where the end user should be warned of something "unexpected" happening.

I think this is likely because of the user error in their ingestion spec, and thus it makes sense to bubble up the user error to their end. The thing is how. To me, from the Druid perspective, the most reasonable way is ending the task with the SUCCEEDED state (because the task technically succeeded with no error) and showing the ingestion metrics, such as # of rows ingested, in the task report, so that users can notice the strangeness of their ingestion job from those metrics. We could probably improve our UI to make this more noticeable.

maytasm · 2021-08-09T16:11:04Z

Merging this in to fix regression. Improvement for logging and UI can be done separately

fix ingestion task failure when no input split to process

0252171

maytasm requested a review from jihoonson August 5, 2021 09:27

asdf2014 added the Area - Ingestion label Aug 5, 2021

jihoonson added Area - Batch Ingestion Bug and removed Area - Ingestion labels Aug 5, 2021

jihoonson reviewed Aug 5, 2021

View reviewed changes

add IT

705f074

fix IT

77e1a3a

suneet-s approved these changes Aug 9, 2021

View reviewed changes

jihoonson approved these changes Aug 9, 2021

View reviewed changes

maytasm merged commit 06bae29 into apache:master Aug 9, 2021

maytasm deleted the REQ-2430 branch August 9, 2021 16:11

clintropolis added this to the 0.22.0 milestone Aug 12, 2021

Conversation

maytasm commented Aug 5, 2021

Description

Uh oh!

suneet-s commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

jihoonson Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

maytasm Aug 5, 2021 • 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!

jihoonson Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

suneet-s Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

jihoonson Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

maytasm Aug 7, 2021

Choose a reason for hiding this comment

Uh oh!

jihoonson Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

maytasm commented Aug 7, 2021

Uh oh!

maytasm commented Aug 7, 2021

Uh oh!

suneet-s 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!

suneet-s commented Aug 9, 2021

Uh oh!

maytasm commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maytasm commented Aug 9, 2021

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

maytasm commented Aug 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

suneet-s commented Aug 5, 2021 •

edited

Loading

maytasm Aug 5, 2021 •

edited

Loading

suneet-s left a comment •

edited

Loading

maytasm commented Aug 9, 2021 •

edited

Loading