storage: Split storage.Stores

[bdarnell]

storage.Stores contains several methods which are only intended for use in tests (LookupReplica, FirstRange, RangeLookup). However, not all tests need them (ones that use a DistSender generally shouldn't, although I was surprised to learn in #8868 that the split snapshot race tests hit this code path), and because Stores.Send "helpfully" calls LookupReplica if RangeID is zero, we could call these methods in production too.

These methods should be removed from Stores and placed on a wrapper object that can be used explicitly by tests that need it, so we can ensure that tests that shouldn't use it don't end up accidentally relying on this behavior.

[tamird]

Would it be sufficient to relocate these methods to helpers_test.go?

On Thu, Sep 1, 2016 at 2:12 AM, Ben Darnell [email protected]
wrote:

storage.Stores contains several methods which are only intended for use
in tests (LookupReplica, FirstRange, RangeLookup). However, not all tests
need them (ones that use a DistSender generally shouldn't, although I was
surprised to learn in #8868
#8868 that the split
snapshot race tests hit this code path), and because Stores.Send
"helpfully" calls LookupReplica if RangeID is zero, we could call these
methods in production too.

These methods should be removed from Stores and placed on a wrapper
object that can be used explicitly by tests that need it, so we can ensure
that tests that shouldn't use it don't end up accidentally relying on this
behavior.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#9014, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPAAXzG2SnTO9AQb4KmxeAe9THAg7ks5qlmzDgaJpZM4JyWT7
.

[bdarnell]

No; many tests rely on the fact that Stores.Send (prod code) calls Stores.LookupReplica (test code) in certain situations (which are not supposed to happen in prod, although there's nothing that guarantees this). We'll have to break up the type, not just move methods around.

I'm also not sure whether all the tests that rely on this are confined to the storage package. I think so, but again something outside the package could be using this.

[tamird]

I'm not sure I understand.

No; many tests rely on the fact that Stores.Send (prod code) calls Stores.LookupReplica (test code) in certain situations (which are not supposed to happen in prod, although there's nothing that guarantees this). We'll have to break up the type, not just move methods around.

Moving Stores.LookupReplica to helpers_test.go would make Stores.Send not compile as long as it calls Stores.LookupReplica.

I'm also not sure whether all the tests that rely on this are confined to the storage package. I think so, but again something outside the package could be using this.

If this is the case then indeed the only solution will be to have a test-only type that lives in a package which we validate is not imported into prod code.

[bdarnell]

It sounds to me like you've summed up the situation pretty well; what part do you not understand? We can't move LookupReplica to a test file as long as it's used in Send. We can't remove the call from Send because many tests will start failing.

[tamird]

My suggestion was to move these functions to a test file as a way of getting the compiler to help guide correcting the problem. In case those functions are called directly from test code, this approach will require less work than extracting a new type.

[bdarnell]

although I was surprised to learn in #8868 that the split snapshot race tests hit this code path

And I was surprised again in #16893 to re-learn that it was still going through this code path. That PR prevents the split snapshot race tests from using this code path, but many other tests in package storage still do.

[benesch]

Fixed by #25061!