KAFKA-7895: fix stream-time reckoning for Suppress (2.1) (#6286)#6325
KAFKA-7895: fix stream-time reckoning for Suppress (2.1) (#6286)#6325bbejeck merged 3 commits intoapache:2.1from vvcephei:KAFKA-7895-fix-suppress-2.1
Conversation
Even within a Task, different Processors have different perceptions of time, due to record caching on stores and in suppression itself, and in general, due to any processor logic that may hold onto records arbitrarily and emit them later. Thanks to this, we can't rely on the whole task existing in the same "instant" of stream-time. The solution is for each processor node that cares about stream-time to track it independently.
vvcephei
left a comment
vvcephei
left a comment
There was a problem hiding this comment.
@guozhangwang @bbejeck Here's the backport to 2.1 of (#6286), when you have a chance to review.
I haven't had a chance to run the tests yet, which we should definitely do before merging, but I just wanted to push my branch and open the PR, since the cherry-pick was so messy.
| private final TimeDefinition<K> bufferTimeDefinition; | ||
| private final BufferFullStrategy bufferFullStrategy; | ||
| private final boolean shouldSuppressTombstones; | ||
| private final boolean safeToDropTombstones; |
There was a problem hiding this comment.
This was an unrelated change in trunk/2.2 to improve the code legibility.
I'm proposing to backport it to keep the Suppress code consistent and improve maintainability.
| * figure out when to forget the fact that we have emitted some result (currently, the | ||
| * buffer immediately forgets all about a key when we emit, which helps to keep it | ||
| * compact). | ||
| */ |
There was a problem hiding this comment.
Likewise, this is part of the Suppress code legibility change.
guozhangwang
changed the title
|
|
retest this please |
|
@vvcephei failed with |
|
Thanks for the reviews, @guozhangwang and @bbejeck . Did that answer your question, @bbejeck ? The reason Thanks for the catch @guozhangwang . That was due to cherry-pick noise. I've pushed a fix for it now, and will monitor the test results. |
Yep, just waiting for a green build at this point. |
|
Hi @bbejeck and @guozhangwang , the tests have passed. Is this ready to merge? Thanks, |
|
Oh, actually no! I need to run the system tests. all system tests passed: http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2019-03-05--001.1551765320--vvcephei--KAFKA-7895-fix-suppress-2.1--dcacd02/report.html |
|
Ok, the system tests passed as well. I think it's ready to merge now. |
|
merged #6325 into |
|
Thanks, @bbejeck ! |
3 participants
Even within a Task, different Processors have different perceptions
of time, due to record caching on stores and in suppression itself,
and in general, due to any processor logic that may hold onto
records arbitrarily and emit them later. Thanks to this, we can't rely
on the whole task existing in the same "instant" of stream-time. The
solution is for each processor node that cares about stream-time to
track it independently.
On the side, backporting some internally-facing code maintainability updates
Committer Checklist (excluded from commit message)