vvcephei commented on pull request #8881: URL: https://github.com/apache/kafka/pull/8881#issuecomment-651188643
Hey again @ConcurrencyPractitioner . I'm sorry to say that I still haven't been able to review. It was a long week last week, resolving a subtle and complex bug around upgrading with Suppression enabled. I did familiarize myself with the issue you pointed out about this PR, and it's a real bummer. I think when I was initially considering how we could implement this without needing any additional serialization calls or lookups, I wasn't thinking about including timestamp semantics in the feature. Unfortunately, it is possible for the "old" value to have a higher timestamp than the current record. If you recall, this was the reason to add that weird `compareValuesAndCheckForIncreasingTimestamp` in the last PR. I think the right approach here would be to do a re-write of the internals so that we actually get the old timestamp forwarded as well, but I do not think we should do that until KIP-478 is complete. Otherwise, the risk is too high. The good news is that I've got a good chunk of that KIP complete already, and I'm just waiting to submit the PR until all the release craziness is done. In the mean time, perhaps we can just focus on the operators for which this isn't a problem. For example, I just looked at KTableAggregate. In that processor, we have to load the prior value AND timestamp already. WDYT about just leaving this PR open for now, and switching to focus on the processors that already load the prior value and timestamp? I'll tag you also on the KIP-478 PRs, so that you can help move that along and unblock yourself, if you like. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
