KAFKA-3452: follow-up -- introduce SesssionWindows#2342
KAFKA-3452: follow-up -- introduce SesssionWindows#2342mjsax wants to merge 3 commits intoapache:trunkfrom
Conversation
mjsax
commented
Jan 11, 2017
mjsax
commented
- TimeWindows represent half-open time intervals while SessionWindows represent closed time intervals
|
@dguy @guozhangwang @enothereska This is a follow up to Session Windows. Because cf #2337 that revealed this problem Potential issues: this PR changes |
|
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): |
dguy
left a comment
dguy
left a comment
There was a problem hiding this comment.
A couple of minor comments.
Also, should we also add tests for UnlimitedWindow and TimeWindow overlap ?
There was a problem hiding this comment.
Can we make these fields final? I know they weren't before, but as you are changing this
There was a problem hiding this comment.
We can. I still have #2337 open that needs to get rebased after this got merged -- will fix it there if ok.
There was a problem hiding this comment.
Does this need to be abstract? Can't we just have the impl of it here? i.e., the SessionWindow and TimeWindow implementations are identical.
There was a problem hiding this comment.
Yes, it should be abstract because Window does not know if boundaries will be inclusive or exclusive. And implementation for SessionWindow and TimeWindow are different IMHO -- or do I miss anything -- at least, both should be different because SessionWindow does have inclusive end time while TimeWindow has exclusive end time.
There was a problem hiding this comment.
Pretty thorough. My only comment, and this is completely subjective so feel free to ignore. I don't really like this style of having lots of assertions in a single test. I'd much rather have single tests that are focused and test just one thing. You can then just read the test names and know how it should behave rather than having to read the code.
There was a problem hiding this comment.
Will change it -- still working to improve my skills to write tests. And you are my "testing hero" right now :)
|
Updated. Test for |
|
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): |
|
@mjsax - looks like there is a checkstyle failure |
|
@dguy @guozhangwang @enothereska Updated. |
|
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): |
- TimeWindows represent half-open time intervals while SessionWindows represent closed time intervals
mjsax
force-pushed
the
kafka-3452-session-window-follow-up
branch
from
c348e03 to
6c3d5af
Compare
|
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): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Failing tests are unrelated. |
|
retest this please |
|
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): |
|
LGTM and merged to trunk. |
- TimeWindows represent half-open time intervals while SessionWindows represent closed time intervals Author: Matthias J. Sax <matthias@confluent.io> Reviewers: Damian Guy <damian.guy@gmail.com>, Guozhang Wang <wangguoz@gmail.com> Closes apache#2342 from mjsax/kafka-3452-session-window-follow-up
4 participants
