Remove unused key.Destination.IsUnique()#7565
Merged
systay merged 1 commit intovitessio:masterfrom Mar 1, 2021
Merged
Conversation
This function is never used. It is also misleading. "IsUnique returns true if this is a single destination". As currently implemented, a key.Destination will return true for this method if it represents a single object of some type, as opposed to an array of objects of some type. It is unclear that this is a useful concept. key.DestinationExactKeyRange and key.DestinationKeyRange both return true always, even though a KeyRange can represent multiple KeyspaceIDs, and therefore might cover multiple shards. Since the IsUnique() interface doesn't accept any information about the topology, the only safe return value would be false, if we wanted IsUnique() to represent a useful piece of information about how the query might end up being served. If this concept needed to be re-introduced, I would suggest one of two approaches: - Introduce new functions IsUniqueKeyspaceID() and IsGuaranteedUniqueShard(). IsUniqueKeyspaceID() would only return true for a DestinationKeyspaceID, or a DestinationKeyspaceIDs that contains 0-1 unique KeyspaceIDs. IsGuaranteedUniqueShard() would return true when IsUniqueKeyspaceID() returns true, or for 0-1 shard names. IsGuaranteedUniqueShard() would always return false for all types of KeyRange destinations. - In addition to the first approach (or instead of IsGuaranteedUniqueShard()), introduce new function IsUniqueShard([]*topodatapb.ShardReference). This would behave the same as IsGuaranteedUniqueShard(), except that for KeyRange destinations, it could potentially return true if the topo data indicates that the KeyRange(s) fit within a single shard. Signed-off-by: Jordan Moldow <jmoldow@alum.mit.edu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This function is never used.
It is also misleading. "IsUnique returns true if this is a
single destination". As currently implemented, a key.Destination
will return true for this method if it represents a single
object of some type, as opposed to an array of objects of some
type. It is unclear that this is a useful concept.
key.DestinationExactKeyRange and key.DestinationKeyRange both
return true always, even though a KeyRange can represent
multiple KeyspaceIDs, and therefore might cover multiple shards.
Since the IsUnique() interface doesn't accept any information
about the topology, the only safe return value would be false,
if we wanted IsUnique() to represent a useful piece of
information about how the query might end up being served.
If this concept needed to be re-introduced, I would suggest one
of two approaches:
Introduce new functions IsUniqueKeyspaceID() and
IsGuaranteedUniqueShard(). IsUniqueKeyspaceID() would only
return true for a DestinationKeyspaceID, or a
DestinationKeyspaceIDs that contains 0-1 unique KeyspaceIDs.
IsGuaranteedUniqueShard() would return true when
IsUniqueKeyspaceID() returns true, or for 0-1 shard names.
IsGuaranteedUniqueShard() would always return false for all
types of KeyRange destinations.
In addition to the first approach (or instead of
IsGuaranteedUniqueShard()), introduce new function
IsUniqueShard([]*topodatapb.ShardReference). This would behave
the same as IsGuaranteedUniqueShard(), except that for KeyRange
destinations, it could potentially return true if the topo data
indicates that the KeyRange(s) fit within a single shard.
Signed-off-by: Jordan Moldow jmoldow@alum.mit.edu
Checklist
Impacted Areas in Vitess
Components that this PR will affect: