You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "Jonathan Ellis (Issue Comment Edited) (JIRA)" <ji...@apache.org> on 2011/11/19 00:53:01 UTC
[jira] [Issue Comment Edited] (CASSANDRA-1034) Remove assumption
that Key to Token is one-to-one
[ https://issues.apache.org/jira/browse/CASSANDRA-1034?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13153240#comment-13153240 ]
Jonathan Ellis edited comment on CASSANDRA-1034 at 11/18/11 11:52 PM:
----------------------------------------------------------------------
{noformat}
+ public static final DecoratedKey minKey = new DecoratedKey(partitioner.getMinimumToken(), false);
{noformat}
I think I'd rather have these in the partitioner. (I know partitioner is cluster-global right now but it still feels wrong to "hoist" something partitioner dependent out and make it static final.)
{noformat}
+ assert token != null && key != null;
{noformat}
This feels odd when we go ahead and construct DKs with null key anyway in the other constructor.
*Important*: I think my biggest problem with this patch is that a DK may or may not have a key that when given to the partitioner, results in the Token in the DK. And there's nothing to show that is the case, except that key == null or Empty. So we're still pretending a Token "is" a key, we've just made it more complicated. Could we update the methods for whose benefits we're performing the Token -> DK conversion, to accept RingPosition instead?
{noformat}
+ return token.hashCode() + (key == null ? 0 : key.hashCode());
{noformat}
I don't see a good reason to not use a "real" hashcode implementation (Objects.hashCode is useful here).
{noformat}
+ // null is used as a 'end of range' marker, so DK(t, k) is always before DK(t, null) unless k == null
{noformat}
Still not a huge fan of using null to mean end of range, but I guess I don't have a better suggestion. There's clearly a lot of places in this patch where it's causing special case ugliness though, independent of its status as "max."
{noformat}
+ // minimunKey, see Token.upperBoundKey()
{noformat}
typo. (both occurrences.)
{noformat}
- T min = (T)current.partitioner.getMinimumToken();
+ T min = (T)current.left.minimumValue(current.partitioner);
{noformat}
I think the positives of making this Generic are outweighed by the negative of implying that minimum value for partitioner X depends on the RingPosition that is returning it. I think I'd rather accept the casting ugliness of having a Partitioner method that does instanceof checks to return the appropriate type.
*Serializer code*: How does DK, AB, etc. code deal w/ backwards compatibility issues? Looks like some (AES) can get by with saying "we don't support mixed-version streaming" but others (IndexScanCommand) cannot.
{noformat}
+ assert left.compareTo(right) <= 0 || right.isMinimum(partitioner) : "[" + left + "," + right + "]";
{noformat}
What if we added a Partitioner reference so we could just ask isMinimum()?
was (Author: jbellis):
{noformat}
+ public static final DecoratedKey minKey = new DecoratedKey(partitioner.getMinimumToken(), false);
{noformat}
I think I'd rather have these in the partitioner. (I know partitioner is cluster-global right now but it still feels wrong to "hoist" something partitioner dependent out and make it static final.)
{noformat}
+ assert token != null && key != null;
{noformat}
This feels odd when we go ahead and construct DKs with null key anyway in the other constructor.
*Important*: I think my biggest problem with this patch is that a DK may or may not have a key that when given to the partitioner, results in the Token in the DK. And there's nothing to show that is the case, except that key == null or Empty. So we're still pretending a Token "is" a key, we've just made it more complicated. Could we update the methods for whose benefits we're performing the Token -> DK conversion, to accept RingPosition instead?
{noformat}
+ return token.hashCode() + (key == null ? 0 : key.hashCode());
{noformat}
I don't see a good reason to not use a "real" hashcode implementation (Objects.hashCode is useful here).
{noformat}
+ // null is used as a 'end of range' marker, so DK(t, k) is always before DK(t, null) unless k == null
{noformat}
Still not a huge fan of using null to mean end of range, but I guess I don't have a better suggestion. There's clearly a lot of places in this patch where it's causing special case ugliness though, independent of its status as "max."
{noformat}
+ // minimunKey, see Token.upperBoundKey()
{noformat}
typo. (both occurrences.)
{noformat}
+ public T minimumValue(IPartitioner partitioner);
{noformat}
{noformat}
- T min = (T)current.partitioner.getMinimumToken();
+ T min = (T)current.left.minimumValue(current.partitioner);
{noformat}
I think the positives of making this Generic are outweighed by the negative of implying that minimum value for partitioner X depends on the RingPosition that is returning it. I think I'd rather accept the casting ugliness of having a Partitioner method that does instanceof checks to return the appropriate type.
*Serializer code*: How does DK, AB, etc. code deal w/ backwards compatibility issues? Looks like some (AES) can get by with saying "we don't support mixed-version streaming" but others (IndexScanCommand) cannot.
{noformat}
+ assert left.compareTo(right) <= 0 || right.isMinimum(partitioner) : "[" + left + "," + right + "]";
{noformat}
What if we added a Partitioner reference so we could just ask isMinimum()?
> Remove assumption that Key to Token is one-to-one
> -------------------------------------------------
>
> Key: CASSANDRA-1034
> URL: https://issues.apache.org/jira/browse/CASSANDRA-1034
> Project: Cassandra
> Issue Type: Bug
> Reporter: Stu Hood
> Assignee: Sylvain Lebresne
> Priority: Minor
> Fix For: 1.1
>
> Attachments: 0001-Generify-AbstractBounds.patch, 0002-Remove-assumption-that-token-and-keys-are-one-to-one.patch, 0003-unit-test.patch, 1034_v1.txt, CASSANDRA-1034.patch
>
>
> get_range_slices assumes that Tokens do not collide and converts a KeyRange to an AbstractBounds. For RandomPartitioner, this assumption isn't safe, and would lead to a very weird heisenberg.
> Converting AbstractBounds to use a DecoratedKey would solve this, because the byte[] key portion of the DecoratedKey can act as a tiebreaker. Alternatively, we could make DecoratedKey extend Token, and then use DecoratedKeys in places where collisions are unacceptable.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira