-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-38670][SS] Add offset commit time to streaming query listener #35985
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add [SS]
after [SPARK-38670]
.
We have a stack graph with regarding to the duration in the micro batch in SS UI - I'm not sure the change is reflected on the graph.
Could you please run a e2e manually and make sure the change is also reflected in the SS UI page as well? Please attach the screenshot and the step you've performed on manual test in the section of "How was this patch tested?".
This PR does introduce the user-facing change since we add the new field for durationMs in streaming listener. Could you please update the section "Does this PR introduce any user-facing change?" to describe the change?
Thanks in advance!
@@ -316,6 +316,7 @@ class StreamingQuerySuite extends StreamTest with BeforeAndAfter with Logging wi | |||
assert(query.recentProgress.last.eq(query.lastProgress)) | |||
|
|||
val progress = query.lastProgress | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will revert
Can one of the admins verify this patch? |
@HeartSaVioR thanks for the review. Can you please take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thanks! Merging to master. |
Thanks @jerrypeng for your contribution! I merged this to master. |
What changes were proposed in this pull request?
Add a metric called "commitOffsets" to the StreamingQueryListener
Why are the changes needed?
A good portion of the batch duration is committing offsets at the end of the micro-batch. The timing for this operation is missing from the durationMs metrics. Lets add this metric to have a more complete picture of where the time is going during the processing of a micro-batch
Does this PR introduce any user-facing change?
Yes, an event returned from the StreamingQueryListener will now contain an additional metric:
How was this patch tested?
add a test