Skip to content

reword for clarity#10748

Merged
jihoonson merged 1 commit intoapache:masterfrom
techdocsmith:native-batch
Feb 8, 2021
Merged

reword for clarity#10748
jihoonson merged 1 commit intoapache:masterfrom
techdocsmith:native-batch

Conversation

@techdocsmith
Copy link
Contributor

Fixes awkward wording around the behavior of appendToExsiting.

Description

Removes ambiguity around the "current limitation". Describes the option in terms of the current product functionaltiy.


This PR has:

  • been self-reviewed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The doc should address what happens if you disregard this advice and set appendToExisting: true but don't use dynamic partitioning. (Do you get an error? Does something weird happen?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am still a noob, but I tried setting the partitionspec to hashed and appendToExisting to true. The task succeeded, but when I check the payload it looks like appendToExisting was set to false

    "ioConfig": {
      "type": "index_parallel",
      "inputSource": {
        "type": "http",
        "uris": [
          "https://static.imply.io/data/wikipedia.json.gz"
        ],
        "httpAuthenticationUsername": null,
        "httpAuthenticationPassword": null
      },
      "inputFormat": {
        "type": "json",
        "flattenSpec": {
          "useFieldDiscovery": true,
          "fields": []
        },
        "featureSpec": {}
      },
      "appendToExisting": false

I'll need to try again maybe and follow up with someone with deeper insight to verify how it is supposed to work in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jihoonson PTAL. Clarified that the task will fail if you use another partitioning type and why. Included the error text in case someone needs to search on that. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually there are a couple cases when something can go wrong.

  • appendToExisting: true, forceGuaranteedRollup: false, partitioning = dynamic: this is the only valid setup.
  • appendToExisting: true, forceGuaranteedRollup: false, partitioning = hashed: error with a message of DynamicPartitionsSpec must be used for best-effort rollup.
  • appendToExisting: true, forceGuaranteedRollup: true, partitioning = dynamic: error with a message of DynamicPartitionsSpec cannot be used for perfect rollup.
  • appendToExisting: true, forceGuaranteedRollup: true, partitioning = hashed/single_dim: error with a message of Perfect rollup cannot be guaranteed when appending to existing dataSources.

I'm not sure it's reasonable to list out all these errors here. It's hard to improve the error message for now because we don't know exactly what users want to do. Maybe it's better to simply say the task will fail.

BTW, forceGuaranteedRollup seems not very useful so we can get rid of it in the future. Once we remove it, then the error message can be simplified to something like appendToExisting cannot be set with hashed partitioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the detailed review @jihoonson . I removed the line break and mention of error. I think your info can go into a troubleshooting topic on the forum

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to break the line here? This actually breaks the table (https://github.com/apache/druid/blob/9331331b857419c6ca45c15a1852058355019c87/docs/ingestion/native-batch.md#ioconfig). You need to add <br/> to break the line.

Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

The new doc looks good to me, but please apply the same change to the other place as well. Also please fix the typo reported in the CI.

      721 | and log the following error: "forceGuranteedRollup must be set".   |false|no| 

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to apply the same change here as well?

Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

+1 after Doc CI.

@jihoonson
Copy link
Contributor

The doc CI passed. I'm merging this PR.

@jihoonson jihoonson merged commit 41a0c1d into apache:master Feb 8, 2021
@clintropolis clintropolis added this to the 0.22.0 milestone Aug 12, 2021
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.

4 participants