cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joshua McKenzie (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-8385) Clean up generics in uses of AbstractType
Date Mon, 23 May 2016 19:56:13 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-8385?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15296975#comment-15296975
] 

Joshua McKenzie commented on CASSANDRA-8385:
--------------------------------------------

* AbstractType.toJsonString includes protocolVersion, however I did not see that parameter
used in any of the implementers.
* For the various valueLengthIfFixed implementations, I think it might make sense to relax
scope on TypeSizes and reference them instead of us encoding that in 2 separate places.
* The naming of "CollectionType.Kind.type" doesn't seem clear to me (fully acknowledging that
you simply made a method of what was previously a member variable with the same name). While
we have "Kind"'s littered throughout the code-base, the meaning of "a type of Kind of CollectionType"
doesn't give a lot of helpful information to reason about and I think this is an opportunity
to fix at least one instance of a widely overloaded name in the code-base. Perhaps naming
it to "classType" to at least denote the context of the type in the name?
* Since neither {{CollectionType}} nor {{ConcreteType}} have implementatoins of {{getSerializer}},
I don't follow the comment in {{ConcreteCollectionType}} of providing a more specific type
than the one in either to resolve a type conflict.
* It looks like we're losing compile-time type-checking in {{ConcreteType.decompose}} relative
to our previous implementation in {{AbstractType.decompose}}. We're still protected at runtime
via {{type.cast}} and, in general, I think the pros outweight the cons here, but thought I'd
point it out in case I was misunderstanding something here.
* {{ConcreteType.isFrozenCollection}} is unused. {{CreateIndexStatement.validate}} is likely
where this method was intended to be referenced as its duplicating this logic.
* The javadoc for the ConcreteType could use some elaboration compared to just re-using the
old javadoc from AbstractType as it is no longer accurate nor representative of the fullness
of what the class does.
* nit: Some grammatical errors in comment for {{AbstractType.compareForCQL}}. Should read:
"this ignore(s) ReversedType", "to be use(d)"
* nit: import order in ConcreteType.java is off

> Clean up generics in uses of AbstractType
> -----------------------------------------
>
>                 Key: CASSANDRA-8385
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-8385
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: Local Write-Read Paths
>            Reporter: Branimir Lambov
>            Assignee: Branimir Lambov
>            Priority: Trivial
>         Attachments: 8385.patch
>
>
> Almost all uses of AbstractType are from code that doesn't know or care what the specific
type is and would be better served by a non-generic version of the concept.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message