-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Change type for UnboundedReaderMaxReadTimeSec #31037
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
Change type for UnboundedReaderMaxReadTimeSec #31037
Conversation
|
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey added as fallback since no labels match configuration Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
CC: @kennknowles |
| void setReaderCacheTimeoutSec(Integer value); | ||
|
|
||
| /** The max amount of time an UnboundedReader is consumed before checkpointing. */ | ||
| @Description( |
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.
Mentioning that the representation can be in fractions of seconds is probably a good call-out here
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.
Good point. added additional comment.
| new Duration(beforeReading, afterReading).getStandardSeconds(), | ||
| lessThanOrEqualTo(maxReadSec + 1)); | ||
| new Duration(beforeReading, afterReading).getMillis(), | ||
| lessThanOrEqualTo((long) (maxReadSec * 1000L))); |
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.
I think you're short a second on this comparison looking at the old unit test, although I do not know if the increased resolution to milliseconds renders the +1 second useless or not
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.
good catch. fixed.
jrmccluskey
left a comment
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.
LGTM
This reverts commit bb310e7.
UnboundedReader is controlled with UnboundedReaderMaxReadTimeSec and UnboundedReaderMaxElements.
For sub second latency user has to set UnboundedReaderMaxElements to 1 which is not flexible as there may be multiple situation where there are more elements.
In this change UnboundedReaderMaxReadTimeSec becomes double. Change is backward compatible.
Unlike #31036, this doesn't introduce additional flag.