Merged
Conversation
The prior wording made it sound like segmentGranularity, queryGranularity, and rollup are always required for granularitySpec. They are not required, but they are strongly recommended. The adjusted wording hopefully does a better job of making that clear.
kfaraz
approved these changes
May 9, 2023
Contributor
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Minor suggestions, but they are optional.
| Compaction tasks fetch all [relevant segments](compaction.md#compaction-io-configuration) prior to launching any subtasks, | ||
| _unless_ the following items are all set. It is strongly recommended to set all of these items to maximize performance | ||
| and minimize disk usage of the `compact` tasks launched by auto-compaction: | ||
| _unless_ the following items are all set. It is strongly recommended to set all of these items to non-null values to |
Contributor
There was a problem hiding this comment.
Maybe use the word field instead of item.
Contributor
There was a problem hiding this comment.
I think for JSON prefer properties over item.
| maximize performance and minimize disk usage of the `compact` tasks launched by auto-compaction: | ||
|
|
||
| - [`granularitySpec`](compaction.md#compaction-granularity-spec). All three values must be set to non-null values: `segmentGranularity`, `queryGranularity`, and `rollup`. | ||
| - [`granularitySpec`](compaction.md#compaction-granularity-spec), including all three of `segmentGranularity`, `queryGranularity`, and `rollup` |
Contributor
There was a problem hiding this comment.
Slight rephrase:
Suggested change
| - [`granularitySpec`](compaction.md#compaction-granularity-spec), including all three of `segmentGranularity`, `queryGranularity`, and `rollup` | |
| - [`granularitySpec`](compaction.md#compaction-granularity-spec), with non-null values for each of `segmentGranularity`, `queryGranularity`, and `rollup`. |
| @@ -116,10 +116,10 @@ The following properties are automatically set by the Coordinator: | |||
| * `context`: Set according to the user-provided `taskContext`. | |||
|
|
|||
| Compaction tasks fetch all [relevant segments](compaction.md#compaction-io-configuration) prior to launching any subtasks, | |||
Contributor
There was a problem hiding this comment.
Suggested change
| Compaction tasks fetch all [relevant segments](compaction.md#compaction-io-configuration) prior to launching any subtasks, | |
| Compaction tasks typically fetch all [relevant segments](compaction.md#compaction-io-configuration) prior to launching any subtasks, |
| Compaction tasks fetch all [relevant segments](compaction.md#compaction-io-configuration) prior to launching any subtasks, | ||
| _unless_ the following items are all set. It is strongly recommended to set all of these items to maximize performance | ||
| and minimize disk usage of the `compact` tasks launched by auto-compaction: | ||
| _unless_ the following items are all set. It is strongly recommended to set all of these items to non-null values to |
Contributor
There was a problem hiding this comment.
Suggested change
| _unless_ the following items are all set. It is strongly recommended to set all of these items to non-null values to | |
| _unless_ the following properties are all set to non-null values. It is strongly recommended to set them to |
cs minor edit to comment.
vtlim
approved these changes
May 11, 2023
sergioferragut
pushed a commit
to sergioferragut/druid
that referenced
this pull request
Jul 21, 2023
* Clarify compaction docs. The prior wording made it sound like segmentGranularity, queryGranularity, and rollup are always required for granularitySpec. They are not required, but they are strongly recommended. The adjusted wording hopefully does a better job of making that clear. * Fix link. * Wording adjustments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The prior wording made it sound like segmentGranularity, queryGranularity, and rollup are always required for granularitySpec. They are not required, but they are strongly recommended. The adjusted wording hopefully does a better job of making that clear.