[SPARK-24697][SS] Fix the reported start offsets in streaming query progress #21744
Conversation
In ProgressReporter for streams, we use the `committedOffsets` as the startOffset and `availableOffsets` as the end offset when reporting the status of a trigger in `finishTrigger`. This is a bad pattern that has existed since the beginning of ProgressReporter and it is bad because its super hard to reason about when `availableOffsets` and `committedOffsets` are updated, and when they are recorded. Case in point, this bug silently existed in ContinuousExecution, since before MicroBatchExecution was refactored. The correct, as well as, surgical fix it to record the offsets explicitly. This PR adds a simple method which is explicitly called from MicroBatch/ContinuousExecition before updating the `committedOffsets`. Added new tests
tdas
changed the title
|
@arunmahadevan I think this explicit specification of offsets to the ProgressReporter makes the control flow easier to follow. Will make future modification easier to reason about. |
|
Test build #92838 has finished for PR 21744 at commit
|
|
jenkins retest this |
|
@tdas logically this is similar to #21673. Yes it makes the control flow better and LGTM. Overall the progress reporter is still tightly coupled with the internals of StreamExecution and the complexity can be reduced by making the ProgressReporter orthogonal to StreamExecution (composition vs inheritance). Anyways that can be handled separately. |
|
jenkins retest this please |
|
@arunmahadevan I agree that this can be refactored later. I was trying to do that, and then realized that it does not make sense to do that in the same PR as this bug fix. thank you for reviewing. |
|
Test build #92844 has finished for PR 21744 at commit
|
3 participants
What changes were proposed in this pull request?
In ProgressReporter for streams, we use the
committedOffsetsas the startOffset andavailableOffsetsas the end offset when reporting the status of a trigger infinishTrigger. This is a bad pattern that has existed since the beginning of ProgressReporter and it is bad because its super hard to reason about whenavailableOffsetsandcommittedOffsetsare updated, and when they are recorded. Case in point, this bug silently existed in ContinuousExecution, since before MicroBatchExecution was refactored.The correct fix it to record the offsets explicitly. This PR adds a simple method which is explicitly called from MicroBatch/ContinuousExecition before updating the
committedOffsets.How was this patch tested?
Added new tests