storage: DRY {client_,}range_tree_test.go

[tamird]

Requested in #5839: these two files mostly test the same things at different levels - it'd be good to abstract over the backing storage and have a single test runner that exercises both.

Seems like a good starter task.

[petermattis]

Or we could remove the range tree code.

[tamird]

Yep, that's also an option, though we've had trouble reaching consensus on that in the past.

[petermattis]

The distsql folks should weigh in. @RaduBerinde @andreimatei do you have any intention to use the range tree code?

[RaduBerinde]

None that I know of - nothing I can think of that can't be done just as well from the meta range structures.

[BramGruneir]

I'll be unhappy if it's gone, but I'll kill it if there's no need for it.

[petermattis]

FYI, here was the previous issue discussing this: https://github.com/cockroachdb/cockroach/issues/3739

[tbg]

We have this discussion every couple of months. My reasoning for keeping them around was "they don't hurt", but having this discussion and random refactoring every now and then does hurt. I'm ok with removing the code. We're very unlikely to use it for a while and once we do, chances are it can be brought back easily.

On Mon, Jun 27, 2016 at 5:22 PM Peter Mattis notifications@github.com wrote:

FYI, here was the previous issue discussing this: #3739 https://github.com/cockroachdb/cockroach/issues/3739

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cockroachdb/cockroach/issues/7495#issuecomment-228879594, or mute the thread https://github.com/notifications/unsubscribe/AE135MQyYlK3HvGO5ZWERPcYkFlBLR6eks5qQD8dgaJpZM4I_gEC .

-- Tobias

[andreimatei]

The range tree is used by this lookupReplica guy, right? https://github.com/cockroachdb/cockroach/blob/553c0e82341c99bf0a3dc782f64dfbf213e72700/storage/stores.go#L198 That function is pretty useful in tests (for example through the Stores.Send() which uses it to fill in missing routing info). Would we replace this somehow?

[tamird]

This is about storage/range_tree.go, which is not used anywhere (except (*Replica).AdminSplit, which maintains the range tree itself).

[petermattis]

The range tree code will be a year old come August without any usage.

[BramGruneir]

The only complication with removing it, is we would need a data migration to remove it.

[petermattis]

@BramGruneir Why would you need a data migration? We can leave the old structures in place and stop updating them.

[BramGruneir]

Yeah, we just had that discussion. It would be a badge of honour if your cluster still had some of the range tree in it.

[a6802739]

so we can remove range_tree.go and range_test.go. In function adminsplit and adminmerge we should also remove the operate with rangetree, right?

[petermattis]

@a6802739 Yes, you would remove the maintenance of the range tree from splits and merges.