KAFKA-20603 : Replace MemoryLRUCache restoring flag with restore lifecycle hooks

[muralibasani]

Ref : https://issues.apache.org/jira/browse/KAFKA-20603

Only refactoring.

Replace MemoryLRUCache restoring flag with onRestoreStart/onRestoreEnd lifecycle hooks on internal RecordBatchingStateRestoreCallback

[UladzislauBlok]

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]

imo this is anti pattern, and we shouldn't test so low level functions. "Test behavior, not implementation"

[muralibasani]

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

kafka/streams/src/main/java/org/apache/kafka/streams/state/internals/MemoryLRUCache.java

Line 98 in fa097fa

restoring = true;

Let me know.