Fix 15271

[serathius]

Fixes #15271
cc @ahrtr @ptabor

[serathius]

cc @lavacat

[serathius]

Proposed followup: ensure that watchableStorage tests don't access storage method.

[serathius]

Another possible improvement is to consider using mutative testing to verify existing unit testing for issues like this.

[ahrtr]

Found problem with watchableStore tests. They don't call their own methods, but ones from underlying storage. As result they don't trigger same watch notification code as ones done by apply flow.

The second commit makes no sense to me. Note the watchableStore just adds a watchable layer on top of the underlying store, why do we encapsulate the KV API again? If you see any issue in unit test, shouldn't we fix test, not the production code?

[serathius]

Found problem with watchableStore tests. They don't call their own methods, but ones from underlying storage. As result they don't trigger same watch notification code as ones done by apply flow.

The second commit makes no sense to me. Note the watchableStore just adds a watchable layer on top of the underlying store, why do we encapsulate the KV API again? If you see any issue in unit test, shouldn't we fix test, not the production code?

Problem is that watchableStore exposes two ways to do writes via s.Put(...) and via s.Write(...).Put(...) (symmetrical for delete). Only the second case goes through proper watchableStore code. First case goes directly to storage instance underneath so it doesn't trigger proper watch notification.

Finding the issue required two things:

• Watching for all events within 1 second, instead of first event. Thus we can find duplication in event.
• Changing the Put call to go to watchableStore and not store underneath. Reason was that notify function that had the bug, was not called in tests at all because all Put requests went to store.

I would argue that this is not a unit test as it tests multiple layers (watchableStore, store and backend) working together. I consider it a bug that testing of watchableStore called Put method not from watchableStore, but from store.

Aside of the issue with test, I find it confusing and weird that calling watchableStore.Put(...) would not notify watchers.
cc @mitake

[ahrtr]

Note neither watchableStore nor the underlying store expose the DeleteRange & Put method. Instead, only interface WriteView exposes them.

• Watching for all events within 1 second, instead of first event. Thus we can find duplication in event.

This is a valid point.

[ahrtr]

#15492 (comment)

[lavacat]

Thanks for adding tests.

I think @ahrtr has a point. When I tried running your updated test without new watchableStore.Put, it passed. When stepping through in debug mode, it goes to writeView.Put and notify is triggered in watchableStoreTxnWrite.End()

[serathius]

Problem is that writeView is a sub-struct of store and store is sub-struct watchableStore. This means that watchableStore can be working even though it was not incorrectly initialized. For correct functioning writeView needs to be reconfigured to point to watchableStore.

This happens in tests where we create watchableStore struct without redirecting writeView to it. This is still very error prone because mvcc package does a lot of usage of private fields/methods. I agree that proposed change in this PR is not optimal, so I will create separate PR to clean up the golang api.

[serathius]

PTAL @ptabor @ahrtr

[ahrtr]

LGTM

Thanks