-
Notifications
You must be signed in to change notification settings - Fork 14.5k
KAFKA-4851: only search available segments during Segments.segments(from, to) #2645
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
Conversation
@guozhangwang @enothereska @mjsax - found this today when someone on the user list reported strange behaviour with Session Windows. Found it performed terribly with caching enabled, but performed well with it turned off. Eventually narrowed it down to calling |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
final long segFrom = segmentId(Math.max(0L, timeFrom)); | ||
final long segTo = segmentId(Math.min(currentSegmentId * segmentInterval, Math.max(0, timeTo))); | ||
final long segFrom = Math.max(minSegmentId, segmentId(Math.max(0L, timeFrom))); | ||
final long segTo = Math.min(currentSegmentId, segmentId(Math.min(currentSegmentId * segmentInterval, Math.max(0, timeTo)))); |
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: shall we rename currentSegmentId
to sth. like latestSegmentId
or maxSegmentId
?
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.
sure - makes sense
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.
One very minor comment, otherwise LGTM.
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Jenkins failures seem to be related to https://issues.apache.org/jira/browse/KAFKA-4841. There is a fix ongoing: #2641 |
…rom, to) restrict the locating of segments in `Segments#segments(..)` to only the segments that are currently available, i.e., rather than searching the hashmap for many segments that don't exist. Author: Damian Guy <[email protected]> Reviewers: Guozhang Wang <[email protected]> Closes #2645 from dguy/session-windows-testing (cherry picked from commit 146edd5) Signed-off-by: Guozhang Wang <[email protected]>
Merged to trunk and cherry-picked to 0.10.2. |
restrict the locating of segments in
Segments#segments(..)
to only the segments that are currently available, i.e., rather than searching the hashmap for many segments that don't exist.