[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[ https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15497491#comment-15497491 ] Shawn Lavelle commented on CASSANDRA-9901: -- This may or may not be the right place for this, but ... I'm posting for awareness. This change converted two anonymous classes into Lambda expressions. One in AbstractType and one in MapSerializer. When I attempt to use any of the Cassandra 3.0.x lines (through 9 snapshot that I compiled), I get runtime class not found exceptions looking for AbstractType$Lambda$$2. I was able to replace the lambda with an anonymous class and remove the exceptions (that's how I found the MapSerializer doing the same thing) and then the code ran. There seems to be a known bug with Javac under 1.8.0_25 and fixed in 1.8.0_45 with lambda's losing type information during compile? Might be related. If there's already a ticket for this, please forgive me, but I couldn't find it. I wanted to post this here in case anyone else encountered it and their googles were as fruitless as mine. > Make AbstractType.isByteOrderComparable abstract > > > Key: CASSANDRA-9901 > URL: https://issues.apache.org/jira/browse/CASSANDRA-9901 > Project: Cassandra > Issue Type: Improvement >Reporter: Benedict >Assignee: Benedict > Fix For: 3.0 beta 2 > > Attachments: C2 compilation output > > > I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably > sure we agreed to make this method abstract, put some javadoc explaining we > may require fields to yield true in the near future, and potentially log a > warning on startup if a user-defined type returns false. > This should make it into 3.0, IMO, so that we can look into migrating to > byte-order comparable types in the post-3.0 world. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[ https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14720308#comment-14720308 ] Ariel Weisberg commented on CASSANDRA-9901: --- +1 Make AbstractType.isByteOrderComparable abstract Key: CASSANDRA-9901 URL: https://issues.apache.org/jira/browse/CASSANDRA-9901 Project: Cassandra Issue Type: Improvement Components: Core Reporter: Benedict Assignee: Benedict Fix For: 3.0 beta 2 Attachments: C2 compilation output I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably sure we agreed to make this method abstract, put some javadoc explaining we may require fields to yield true in the near future, and potentially log a warning on startup if a user-defined type returns false. This should make it into 3.0, IMO, so that we can look into migrating to byte-order comparable types in the post-3.0 world. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[ https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14717408#comment-14717408 ] Ariel Weisberg commented on CASSANDRA-9901: --- In ClusteringComparator.compareComponent can you invoke AbstractType.compare? A little bit more DRY and since it is final it can be inlined. Truthfully I could +1 either. I am more bugged by the lack of direct unit tests for ClusteringComparator and the AbstractType hierarchy. Where is this based off of? The tests don't look as healthy as trunk or 3.0? Make AbstractType.isByteOrderComparable abstract Key: CASSANDRA-9901 URL: https://issues.apache.org/jira/browse/CASSANDRA-9901 Project: Cassandra Issue Type: Improvement Components: Core Reporter: Benedict Assignee: Benedict Fix For: 3.0 beta 2 Attachments: C2 compilation output I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably sure we agreed to make this method abstract, put some javadoc explaining we may require fields to yield true in the near future, and potentially log a warning on startup if a user-defined type returns false. This should make it into 3.0, IMO, so that we can look into migrating to byte-order comparable types in the post-3.0 world. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14717627#comment-14717627
]
Benedict commented on CASSANDRA-9901:
-
I've rebased to latest 3.0, and simply removed the {{isByteOrderComparable}}
from {{ClusteringComparator}} entirely - this will result in some extra
indirection than we have previously incurred (versus 2.1), which I'm not keen
on, but won't contest. Whomever wants to optimise this at a later date can
unpick the utility of eliminating that.
It is worth noting, this particular method is one of _the_ central methods for
the entire codebase, and we have evidence like CASSANDRA-10084 that
inefficiencies here have significant downsides, so we should consider visiting
it. However ultimately I think we need to attack our data model to make real
inroads on our comparison costs. e.g. CASSANDRA-6936, which this ticket is
intended to help enable.
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
Attachments: C2 compilation output
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14711633#comment-14711633
]
Benedict commented on CASSANDRA-9901:
-
We aren't optimising {{compareComponent}}. That would imply this ticket is
sneaking in some positive changes\*, when in fact it is simply avoiding
introducing a negative one. The prior behaviour had one inlining; this would
have resulted in two, and a dramatic increase in the size of the method.
Now, it may be that if we were to measure the effect it would be hard to do so.
However that would itself be largely down to the fact we _never consider these
effects_. Death by a thousand papercuts. Stemming the blood flow from one
papercut doesn't do much. As such I cannot personally subscribe to an ethos of
kicking down the road at least a reasonable consideration of the effects of a
change I'm actively making.
Certainly, if the code were significantly ungainly as a result, investigation
and a follow up ticket might be necessary. Here, the code is just 100% fine,
with barely the faintest hint of ugliness, and the benefit (i.e.
non-detriment) was likely (and demonstrated). I cannot fathom why it should
spawn such discussion.
\*In fact it does sneak in a small improvement, by forcing a conversion to
{{ImmutableList}} in the constructor to permit efficient despatch for the list
get method. However this was not picked out for censure, despite _actually_
falling into the category of unrelated optimisation. Go figure.
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
Attachments: C2 compilation output
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[ https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14711585#comment-14711585 ] Jonathan Ellis commented on CASSANDRA-9901: --- So, Benedict is clearly correct about the compilation effect here. But, I do think Sylvain is right that it is good hygiene to tackle optimization in another ticket. (It's also not clear to me that optimize compareComponent is the right answer to how do we make reads and compaction faster, but that's why we should have that discussion separately.) Make AbstractType.isByteOrderComparable abstract Key: CASSANDRA-9901 URL: https://issues.apache.org/jira/browse/CASSANDRA-9901 Project: Cassandra Issue Type: Improvement Components: Core Reporter: Benedict Assignee: Benedict Fix For: 3.0 beta 2 Attachments: C2 compilation output I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably sure we agreed to make this method abstract, put some javadoc explaining we may require fields to yield true in the near future, and potentially log a warning on startup if a user-defined type returns false. This should make it into 3.0, IMO, so that we can look into migrating to byte-order comparable types in the post-3.0 world. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14711061#comment-14711061
]
Benedict commented on CASSANDRA-9901:
-
bq. I'd left ClusteringComparator.compareComponent as is (before the patch).
This may lead to suboptimal compilation. The result would most likely be two
occurrences of the {{FastByteOperations.compareUnsigned}} calls, with however
much of these can be inlined into the method duplicated (most likely a
significant quantity).
bq. then an assert in compare() is probably better.
I've explicitly moved the assert into the ClusteringComparator constructor
(which is the only call site for the assertion), and have introduced a check to
enforce that the {{compareCustom}} method is not overridden. This way we get
assertions for free, as {{NOT_COMPARABLE}} results in
{{!isByteOrderComparable}}, so we will hit the
{{UnsupportedOperationException}} in this case as a final resort.
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14711223#comment-14711223
]
Benedict commented on CASSANDRA-9901:
-
I completely reject that. If you insist that I perform bytecode analysis to
demonstrate that this pollutes the instruction cache, than I can do so, but I
will not separate the decision from this patch.
As far as I am concerned, making rational decisions based on likelihood of
result when there are limited resources to expend chasing down every proof is
the only sane way to perform development. Furthermore, previously we only had
_one_ such call, since the compare method _did not_ perform
{{FastByteOperations.compareUnsigned}}
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[ https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14711217#comment-14711217 ] Sylvain Lebresne commented on CASSANDRA-9901: - bq. This may lead to suboptimal compilation. In the absence of any evidence that this has any measurable impact (nor, for this matter, than this happen in the first place), and given that this makes the code more convoluted than necessary, I currently consider this premature optimization and maintain my objection to this change. Further, as this ticket is not about performance, I would appreciate if you moved that change to a separate ticket if you wish to push it further. Make AbstractType.isByteOrderComparable abstract Key: CASSANDRA-9901 URL: https://issues.apache.org/jira/browse/CASSANDRA-9901 Project: Cassandra Issue Type: Improvement Components: Core Reporter: Benedict Assignee: Benedict Fix For: 3.0 beta 2 I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably sure we agreed to make this method abstract, put some javadoc explaining we may require fields to yield true in the near future, and potentially log a warning on startup if a user-defined type returns false. This should make it into 3.0, IMO, so that we can look into migrating to byte-order comparable types in the post-3.0 world. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[ https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14711272#comment-14711272 ] Sylvain Lebresne commented on CASSANDRA-9901: - This and past experiences has led me to suspect we have some unreconcilable notions of what sane software development mean. I prefer un-assigning myself from review and let someone else have a go at it. Make AbstractType.isByteOrderComparable abstract Key: CASSANDRA-9901 URL: https://issues.apache.org/jira/browse/CASSANDRA-9901 Project: Cassandra Issue Type: Improvement Components: Core Reporter: Benedict Assignee: Benedict Fix For: 3.0 beta 2 I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably sure we agreed to make this method abstract, put some javadoc explaining we may require fields to yield true in the near future, and potentially log a warning on startup if a user-defined type returns false. This should make it into 3.0, IMO, so that we can look into migrating to byte-order comparable types in the post-3.0 world. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14709292#comment-14709292
]
Sylvain Lebresne commented on CASSANDRA-9901:
-
The approach looks good but I still have a few small remarks:
* I think calling {{checkComparable}} in {{CFMetadata.rebuild}} is enough (no
need to duplicate the call in the builder, since this will end up in rebuild
anyway).
* I'd left {{ClusteringComparator.compareComponent}} as is (before the patch).
{{AbstractType.compare}} is now monomorphic (and we should make it {{final}} to
ensure it stays true), so I'd expect it to be inlined and the indirection to
not cost us anything really. This would avoid the imo slightly ugly form of the
new condition in particular.
* I'd make {{AbstractType.compareCustom()}} throw an
{{UnsupportedOperationException}} by default, thus not requiring it in
{{BYTE_ORDER}} implementations. This going more in the idea of making it easier
for {{BYTE_ORDER}} implementations, since that's what we want to favor. A
small javadoc on that method would be nice too.
* Since we keep a {{isByteOrderComparable}} boolean as a field in
{{AbstractType}}, it'd make more sense to return that in
{{isByteOrderComparable}}. If we want to ensure {{compare()}} is not called for
a {{NOT_COMPARABLE}}, then an assert in {{compare()}} is probably better.
* The comments on {{ComparisonType}} should probably be slightly updated. On
{{BYTE_ORDER}} for instance, the comment should probably just say The values
of this type are compared by their sequence of unsigned bytes, since this is
now enforced. Similarly, {{CUSTOM}} comment needs update.
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14703710#comment-14703710
]
Benedict commented on CASSANDRA-9901:
-
Patch available [here|https://github.com/belliottsmith/cassandra/tree/9901]
* This has kept the {{isByteOrderComparable}} flag in the
{{ClusteringComparator}} as I hadn't factored in the extra cost of performing a
{{List}} lookup in cases where we can avoid it
* In AbstractType we also extract the isByteOrderComparable into a boolean flag
for one less level of indirection when performing this test
* We directly call {{compareCustom}} in {{ClusteringComparator}}, but
everywhere else we rely on the final implementation of {{compare()}}
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14700916#comment-14700916
]
Benedict commented on CASSANDRA-9901:
-
bq. In fact, I'm not even really sure a warning is necessary in the first place.
I'm fine with dropping it.
Otherwise I'm fine with these proposals. Or, if you prefer, I don't mind having
an abstract {{isByteOrderComparable()}} and memoizing the result of it to a
boolean property during construction.
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14700929#comment-14700929
]
Sylvain Lebresne commented on CASSANDRA-9901:
-
bq. Or, if you prefer, I don't mind having an abstract
{{isByteOrderComparable()}} and memoizing the result of it to a boolean
property during construction.
Nah, I'm fine with the enum after all. I do like the option of being able to
declare the type as incomparable to enforce it's never used in a clustering.
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14699879#comment-14699879
]
Sylvain Lebresne commented on CASSANDRA-9901:
-
bq. One of the advantages of using an enum that I did not enumerate was the
possibility of performing more efficient despatch for mixed clustering data
Alright, fair enough. I guess one of the thing that put me off is that
{{COMPARE_COMPARABLE}} sounds really weird/unintuitive to me. If we go with the
{{compareValue}} change I discuss above, then I'd suggest renaming the enum to
something like:
{noformat}
enum ComparisonType { UNCOMPARABLE, BYTE_ORDER, CUSTOM }
{noformat}
and the {{compareValue}} is discussed above could be instead
{{compareCustom()}}. We'd also wouldn't need {{isByteOrderComparable}} I
believe since it would be directly handled by the {{compare}} (which won't be a
virtual call).
bq. Admittedly I haven't confirmed this, but it looks fine to me
I read that too quickly and missed the package check, my bad. I guess it's note
entirely full-proof, but we probably can't do much better short of having a
white-list which would be ugly so I'm fine with that.
bq. I prefer to log more often than less, since there's more chance of it being
spotted. I don't think we rebuild so often - just during schema changes, no?
{{rebuild}} happens every time a {{CFMetaData}} is created and validated, which
means at least on every startup and multiple times per schema change (since
it's called during validation), and that's not counting the case I forget.
A bit of context is also that I strongly suspect that while there is likely
people already using custom types, I don't think there is all that many created
new custom types now that we provide a relatively rich amount of types out of
the box (that was not always the case). So that I'm nore worried about annoying
people that have existing custom types, for which the message is basically
useless since it's not currently actionable and they can't miss the change
anyway since they'll have to update their own code.
In fact, I'm not even really sure a warning is necessary in the first place. As
said, for people already having a custom type, the warning is mostly annoyance.
And for new user than might decide to write a custom type, I think being extra
clear on the javadoc of the {{compareCustom()}} method that you should not
implement it in newly created types would be fair enough warnings (we can
additional add to the {{AbstractType}} javadoc that creating custom subclasses
is frown upon nowadays).
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0 beta 2
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14699798#comment-14699798
]
Benedict commented on CASSANDRA-9901:
-
bq. What I could suggest, on top of
If we were to say instead of and we stuck with the enum, I'd be with you. One
of the advantages of using an enum that I did not enumerate was the possibility
of performing more efficient despatch for mixed clustering data (i.e. with
some byte comparable, some not), especially given we now always consult the
boolean parameter from the class property, since the comparisons of each
clustering column is performed in a different method call now (so if we have a
different class property to consult as cheaply, we may as well do so).
Having two virtual invocations instead of one on this path is a pretty
significant burden we should avoid, however.
bq. From a quick look, it looks like the patch will log warning for every
internal type that is not byte comparable
{code}
+if
(!getClass().getPackage().equals(AbstractType.class.getPackage()))
+logger.warn(Type + this + is not comparable by its
unsigned sequence of raw bytes. A future (major) release of Cassandra may
remove support for such arbitrary comparisons, however upgrade steps will be
provided to ensure a smooth transition.);
{code}
Admittedly I haven't confirmed this, but it looks fine to me, and I'll double
check before we commit.
bq. Also, logging in CFMetadaData.rebuild is going to be more noisy than
necessary
I prefer to log more often than less, since there's more chance of it being
spotted. I don't think we rebuild _so_ often - just during schema changes, no?
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0.0 rc1
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[
https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14699767#comment-14699767
]
Sylvain Lebresne commented on CASSANDRA-9901:
-
From a quick look, it looks like the patch will log warning for every internal
type that is not byte comparable, which is not what we want. Also, logging in
{{CFMetadaData.rebuild}} is going to be more noisy than necessary since that's
called reasonably often. Ideally we'd want to only warn when the type is used
in the first place.
On a less important note, but I'm not a fan of using an enum. I'm not convinced
it'll add clarity for the user, but on the other side, we don't validate that
what the enum said is consistent with what the compare method does which feels
error prone to me. I also find it more clunky (than just making
{{isByteOrderComparable}} abstract) but that's probably more a question of
personal taste.
What I could suggest, on top of making {{isByteOrderComparable}} abstract, is
to create some {{compareValue()}} (or some other name) that would be the
existing {{compare()}}, and the {{compare()}} we actually used would basically
be:
{noformat}
public final int compare(ByteBuffer b1, ByteBuffer b2)
{
return isByteBufferComparable() ? ByteBufferUtil.compareUnsigned(b1, b2) :
compareValue(b1, b2);
}
{noformat}
And {{compareValue}} would be abstract, throwing
{{UnsupportedOperationException}} by default, and only the implementations that
are not bytes comparable would have to provide it.
Make AbstractType.isByteOrderComparable abstract
Key: CASSANDRA-9901
URL: https://issues.apache.org/jira/browse/CASSANDRA-9901
Project: Cassandra
Issue Type: Improvement
Components: Core
Reporter: Benedict
Assignee: Benedict
Fix For: 3.0.0 rc1
I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably
sure we agreed to make this method abstract, put some javadoc explaining we
may require fields to yield true in the near future, and potentially log a
warning on startup if a user-defined type returns false.
This should make it into 3.0, IMO, so that we can look into migrating to
byte-order comparable types in the post-3.0 world.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (CASSANDRA-9901) Make AbstractType.isByteOrderComparable abstract
[ https://issues.apache.org/jira/browse/CASSANDRA-9901?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14641775#comment-14641775 ] Aleksey Yeschenko commented on CASSANDRA-9901: -- Yep, that was the agreement. Log a warning if a custom non-boc type is used as a clustering column. Make AbstractType.isByteOrderComparable abstract Key: CASSANDRA-9901 URL: https://issues.apache.org/jira/browse/CASSANDRA-9901 Project: Cassandra Issue Type: Improvement Components: Core Reporter: Benedict Assignee: Benedict Fix For: 3.0.0 rc1 I can't recall _precisely_ what was agreed at the NGCC, but I'm reasonably sure we agreed to make this method abstract, put some javadoc explaining we may require fields to yield true in the near future, and potentially log a warning on startup if a user-defined type returns false. This should make it into 3.0, IMO, so that we can look into migrating to byte-order comparable types in the post-3.0 world. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
