[
https://issues.apache.org/jira/browse/CASSANDRA-1034?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13153240#comment-13153240
]
Jonathan Ellis commented on CASSANDRA-1034:
-------------------------------------------
{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
