[SPARK-55699][SS][FOLLOWUP] Inconsistent reading of LowLatencyClock when used together with ManualClock#54550
Conversation
HeartSaVioR
changed the title
|
Could you please update the PR title and description to describe the new change in this PR instead of just duplicating the old one? Also it doesn't look like any existing test wasn't failing and we don't add any test here. This looks like just to clarify the time unit for the parameter - that's already actually mentioned in the method doc. I'm OK with further clarification rather than relying on method doc; just want to understand the actual change here. |
eason-yuchen-liu
left a comment
eason-yuchen-liu
left a comment
There was a problem hiding this comment.
I have update the PR description to reflect the actual change. Thanks.
| Thread.sleep(POLL_TIME) | ||
| current = getRecordWithTimestamp | ||
| elapsedTimeMs = (clock.nanoTime() - startReadTime) / 1000 / 1000 | ||
| elapsedTimeMs = clock.getTimeMillis() - startTimeMs |
There was a problem hiding this comment.
@HeartSaVioR This is the main change. Basically, in the last PR, we were substract a millisecond off a nano second which is wrong.
I am not sure why no unit test is failing. Maybe this unveils that we have a limited test coverage.
2 participants
What changes were proposed in this pull request?
In the previous PR, we update the signature of
nextWithTimeoutinSupportsRealTimeRead. However, there was a bug introduced inLowLatencyMemoryStreamwhere we compared millisecond timestamp and nanosecond timestamp directly without conversion.This PR fixes this issue, and renamed the parameter to better prevent such issue from happening.
Why are the changes needed?
Context of the previous PR: There was an issue that RTM tests that use manual clock might stuck because manual clock advancement may happens right in between
LowLatencyReaderWrapgetting the reference time, andnextWithTimeoutin each source getting the start time.nextWithTimeoutmay uses the already advanced time to time its wait. When this happens, since the manual clock may only be advanced once by the test,nextWithTimeoutmay never return, causing test timeout. This only affects LowLatencyMemoryStream when using together with manual clocks.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing unit tests.
Was this patch authored or co-authored using generative AI tooling?
No.