[SPARK-38124][SS][FOLLOWUP] Document the current challenge on fixing distribution of stateful operator #35512
Conversation
This file contains 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
…distribution of stateful operator
viirya
approved these changes
Feb 14, 2022
sunchao
approved these changes
Feb 14, 2022
| @@ -101,6 +101,14 @@ case class ClusteredDistribution( | |||
| * Since this distribution relies on [[HashPartitioning]] on the physical partitioning of the | |||
| * stateful operator, only [[HashPartitioning]] (and HashPartitioning in | |||
| * [[PartitioningCollection]]) can satisfy this distribution. | |||
| * | |||
| * NOTE: This is applied only stream-stream join as of now. For other stateful operators, we have | |||
There was a problem hiding this comment.
"applied only to stream-stream join"?
| * partitionings can satisfy the requirement.) We need to construct the way to fix this with | ||
| * minimizing possibility to break the existing checkpoints. | ||
| * | ||
| * TODO: SPARK-38204 to address above note. |
There was a problem hiding this comment.
nit nit: I saw we usually use the pattern TODO(SPARK-38204)
There was a problem hiding this comment.
We seem to use both, but I see more usages on () so I'll follow it. Thanks!
| * | ||
| * NOTE: This is applied only stream-stream join as of now. For other stateful operators, we have | ||
| * been using ClusteredDistribution, which could construct the physical partitioning of the state | ||
| * in different way. (ClusteredDistribution requires relaxed condition and multiple |
There was a problem hiding this comment.
maybe "in different way (ClusteredDistribution requires) ...": no dot after "way".
HyukjinKwon
approved these changes
Feb 15, 2022
|
Thanks! Merging to master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changes were proposed in this pull request?
This PR proposes to add the context of current challenge on fixing distribution of stateful operator, even the distribution is a sort of "broken" now.
This PR addresses the review comment #35419 (comment)
Why are the changes needed?
In SPARK-38124 we figured out the existing long-standing problem in stateful operator, but it is not easy to fix since the fix may break the existing query if the fix is not carefully designed. Anyone should also be pretty much careful when touching the required distribution. We want to document this explicitly to help others to be careful whenever someone is around the codebase.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Code comment only changes.