Skip to content
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

KAFKA-4851: only search available segments during Segments.segments(from, to) #2645

Closed
wants to merge 2 commits into from

Conversation

dguy
Copy link
Contributor

@dguy dguy commented Mar 6, 2017

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.

@dguy
Copy link
Contributor Author

dguy commented Mar 6, 2017

@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 Segments.segments(0, Long.MAX_VALUE) on each flush. This resulted in about 3 million gets on a ConcurrentHashMap - most of which will miss.

@asfbot
Copy link

asfbot commented Mar 6, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2036/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Mar 6, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2034/
Test PASSed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Mar 6, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2033/
Test FAILed (JDK 7 and Scala 2.10).

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))));
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure - makes sense

Copy link
Contributor

@guozhangwang guozhangwang left a 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.

@asfbot
Copy link

asfbot commented Mar 7, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2049/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Mar 7, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2046/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Mar 7, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2047/
Test FAILed (JDK 8 and Scala 2.12).

@guozhangwang
Copy link
Contributor

Jenkins failures seem to be related to https://issues.apache.org/jira/browse/KAFKA-4841. There is a fix ongoing: #2641

@asfgit asfgit closed this in 146edd5 Mar 7, 2017
asfgit pushed a commit that referenced this pull request Mar 7, 2017
…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 <damian.guy@gmail.com>

Reviewers: Guozhang Wang <wangguoz@gmail.com>

Closes #2645 from dguy/session-windows-testing

(cherry picked from commit 146edd5)
Signed-off-by: Guozhang Wang <wangguoz@gmail.com>
@guozhangwang
Copy link
Contributor

Merged to trunk and cherry-picked to 0.10.2.

@dguy dguy deleted the session-windows-testing branch March 30, 2017 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants