JAVA-1397: Handle duration as native datatype in protocol v5+ by olim7t · Pull Request #801 · apache/cassandra-java-driver

olim7t · 2017-02-10T00:37:43Z

Protocol v5 is still beta, I temporarily added this (not committed) in DurationIntegrationTest to test:

    @Override
    public Cluster.Builder createClusterBuilder() {
        return super.createClusterBuilder().allowBetaProtocolVersion();
    }

olim7t · 2017-02-10T00:38:48Z

I think we should also update DataTypeIntegrationTest and similar, looking into that.

olim7t · 2017-02-10T01:19:42Z

In hindsight, it's better to keep duration tests separate because the type has specific restrictions (e.g. can't be used in collections) so it can't be handled as a "regular" primitive type by DataTypeIntegrationTest.
I've added the logic to exclude it so that things don't break when v5 becomes non-beta.

olim7t · 2017-02-10T01:21:15Z

-    @Test(groups = "unit")
+    @Test(groups = "unit", enabled = false)
    public void should_return_same_elements_with_all_primitive_types_using_latest_protocol_version() {
        assertThat(DataType.allPrimitiveTypes(ProtocolVersion.NEWEST_SUPPORTED)).isEqualTo(DataType.allPrimitiveTypes());


This fails, I'm not sure if we should adapt it or remove it altogether.

I vote for removing it. The method under test – DataType.allPrimitiveTypes(ProtocolVersion) – is itself a test method and the assumption – NEWEST_SUPPORTED should contain all known primitive types – is simply not true anymore.

should_not_return_v4_types_in_all_primitive_types_with_v3 (right above) could be deleted too.

adutra

Approving my own changes \o/

adutra · 2017-02-10T08:34:30Z

-        TINYINT(20, ProtocolVersion.V4),
-        DATE(17, ProtocolVersion.V4),
-        TIME(18, ProtocolVersion.V4);
+        TUPLE(49, ProtocolVersion.V3);


If you re-order by protocolId, why not fully re-order?

Right, I missed a few of them.

adutra · 2017-02-10T08:35:27Z


            @Override
            public boolean apply(DataType dataType) {
-                return protocolVersion.compareTo(dataType.getName().minProtocolVersion) >= 0;


This method is only used in tests, therefore I would suggest that we move it to TestUtils, wdyt? To buy us some time, I will push a commit to do that myself, but feel free to revert if you disagree.

adutra · 2017-02-10T08:38:42Z

-                return protocolVersion.compareTo(dataType.getName().minProtocolVersion) >= 0;
+                // Duration is handled separately in DurationIntegrationTest, because it has specific restrictions (e.g.
+                // not allowed in collections).
+                return dataType.getName() != Name.DURATION &&


Also, I think the "base" method should return all types for a given version, and let individual tests filter out undesired ones, like in DataTypeTest.exclude(DataType).

adutra · 2017-02-10T08:43:33Z

-    @Test(groups = "unit")
+    @Test(groups = "unit", enabled = false)
    public void should_return_same_elements_with_all_primitive_types_using_latest_protocol_version() {
        assertThat(DataType.allPrimitiveTypes(ProtocolVersion.NEWEST_SUPPORTED)).isEqualTo(DataType.allPrimitiveTypes());


I vote for removing it. The method under test – DataType.allPrimitiveTypes(ProtocolVersion) – is itself a test method and the assumption – NEWEST_SUPPORTED should contain all known primitive types – is simply not true anymore.

adutra · 2017-02-10T08:48:20Z

-     * This class is a singleton; to obtain its unique instance,
-     * call {@link #duration()}.
-     */
-    public static class DurationType extends CustomType {


Pushed a small commit to delete this.

adutra · 2017-02-10T09:50:44Z

-     *
-     * @param protocolVersion protocol version to get types for.
-     * @return returns a set of all the primitive types for the given protocolVersion.
-     */


Moved to TestUtils as per my own suggestion.

olim7t · 2017-02-10T17:40:06Z

-    @Test(groups = "unit")
-    public void should_return_same_elements_with_all_primitive_types_using_latest_protocol_version() {
-        assertThat(DataType.allPrimitiveTypes(ProtocolVersion.NEWEST_SUPPORTED)).isEqualTo(DataType.allPrimitiveTypes());
-    }


Removed these two tests, as they cover a method that has been moved to TestUtils.

tolbertam

looks good to me! 👍

olim7t added this to the 3.2.0 milestone Feb 10, 2017

olim7t commented Feb 10, 2017

View reviewed changes

adutra approved these changes Feb 10, 2017

View reviewed changes

adutra reviewed Feb 10, 2017

View reviewed changes

olim7t commented Feb 10, 2017

View reviewed changes

tolbertam approved these changes Feb 10, 2017

View reviewed changes

JAVA-1397: Handle duration as native datatype in protocol v5+

46011fb

olim7t force-pushed the java1397 branch from 1ff0e33 to 46011fb Compare February 10, 2017 18:02

olim7t merged commit a12116a into 3.x Feb 10, 2017

olim7t deleted the java1397 branch February 10, 2017 18:03

Conversation

olim7t commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olim7t commented Feb 10, 2017

Uh oh!

olim7t commented Feb 10, 2017

Uh oh!

olim7t Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tolbertam left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olim7t commented Feb 10, 2017 •

edited

Loading

olim7t Feb 10, 2017 •

edited

Loading