muralibasani commented on PR #22366:
URL: https://github.com/apache/kafka/pull/22366#issuecomment-4536847319
> Thanks for the PR. While code looks okay, I want to check on the proposal
overall. It looks like we just introduced more complicated solution to just
reset flag. Isn't that overkill?
@UladzislauBlok thanks for the review.
Indeed looks like an overkill. The original TODO comment
```
// TODO: this is a sub-optimal solution to avoid logging during restoration.
// in the future we should augment the StateRestoreCallback with onComplete
etc to better resolve this.
```
is actually addressed by a simple try finally inside MemoryLRUCache, then
the flag becomes exception safe and it is not sub-optimal anymore imo.
But currently only MemoryLRUCache reads this flag, so the new design
proposed does not justify it. If we have another consumer/store needs this
flag, design gets justified.
For now, if we want to keep it simple, we can revert this design choice and
introduce a try/finally here
https://github.com/apache/kafka/blob/fa097fa593a2a3da17bd1c234bf140565561a1b8/streams/src/main/java/org/apache/kafka/streams/state/internals/MemoryLRUCache.java#L98
Let me know.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
