cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Ellis (Issue Comment Edited) (JIRA)" <j...@apache.org>
Subject [jira] [Issue Comment Edited] (CASSANDRA-1034) Remove assumption that Key to Token is one-to-one
Date Fri, 18 Nov 2011 23:53:01 GMT

    [ 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

        

Mime
View raw message