JAVA-721: Allow user to register custom type codecs (2.2 branch).

[adutra]

README file:
https://github.com/datastax/java-driver/blob/java721-2.2/features/custom_codecs/README.md

Upgrade Guide:
https://github.com/datastax/java-driver/blob/java721-2.2/upgrade_guide/README.md

[tolbertam]

Any reason why we couldn't upgrade to an even newer version of Guava (i.e. 18) or are there compatibility issues with other libraries?

[adutra]

I haven't tried, but there is no compatibility issues that I am aware of.

[adutra]

Will bump to 18.0; feels like it's now or never.

[tolbertam]

👍 my only worry is that this can bring compatibility issues for users, but I think this is acceptable given this is a major release, and that guava is mostly backwards compatible (and in the places its not it should be easy to work around).

[tolbertam]

Would it be more advisable to recommend calling registerDefaults() first then register(myCodec...) so any custom registered codecs take precedence over the default ones? Or will we not expect it to be normal to override default codecs?

[tolbertam]

Nevermind this. It looks like registerDefaults was removed as this is now done as part of initialization of codec registry. Documentation should be updated to reflect that.

[adutra]

You're right, I need to update the docs that are a bit outdated.

[olim7t]

Let's introduce a builder for this class, like for Policies. This will avoid backward-compatibility issues with the constructors.

[adutra]

Ha, I actually avoided the builder pattern on purpose, because codec registry instances are mutable. But note that the class is final, and users are not meant to mess with it (they can play with TypeCodecs now, but that's all).

[tolbertam]

This felt kind of weird because i'm changing my CodecRegistry after creating the Cluster, but it seems necessary because we need the ProtocolVersion among other things in order to properly use Tuples. I noticed that the same needs to be done for UDTs (like TypeCodecUDTIntegrationTest)

[adutra]

Yes :( there is a chicken-and-egg problem here, but I don't see any better way.

[adutra]

Although, I changed the constructor to take a TupleCodec as its only argument; at least I think that the instantiation of TimeZonePreservingDateTimeCodec is a bit less convoluted this way.

[tolbertam]

Pushed a few changes to fix some inconsistencies in behavior between this branch and 2.2. I've documented my commits with details of what I changed. Also added some additional validation based on some of these changes. If I did anything incorrect or some of these inconsistencies are intentional feel free to back out.

[tolbertam]

It looks like there may be a bug when determining what codec to use in Uitls.appendValue (used by QueryBuilder). These two tests test pass most of the same, but sometimes they fail because it appears that they pick up the wrong LongCodec (TimeCodec instead of CounterCodec). With the changes in CodecRegistry to use Cache.asMap() (instead of the CopyOnWriteArrayList) the ordering of codecs is not predictable, so at times it'll choose TimeCodec and other times not, although previously this would have been a problem if CounterCodec was registered first. It looks like this was already known as there is a comment in Utils.isForceAppendToQueryString that is a TODO but thought I'd bring up just in case :)

batchCounterTest(com.datastax.driver.core.querybuilder.QueryBuilderTest)  Time elapsed: 0.012 sec  <<< FAILURE!
java.lang.AssertionError: expected [BEGIN COUNTER BATCH USING TIMESTAMP 42 UPDATE foo SET a=a+1;UPDATE foo SET b=b+2;UPDATE foo SET c=c+3;APPLY BATCH;] but found [BEGIN COUNTER BATCH USING TIMESTAMP 42 UPDATE foo SET a=a+'00:00:00.000000001';UPDATE foo SET b=b+'00:00:00.000000002';UPDATE foo SET c=c+'00:00:00.000000003';APPLY BATCH;]
    at org.testng.Assert.fail(Assert.java:94)
    at org.testng.Assert.failNotEquals(Assert.java:494)
    at org.testng.Assert.assertEquals(Assert.java:123)
    at org.testng.Assert.assertEquals(Assert.java:176)
    at org.testng.Assert.assertEquals(Assert.java:186)
    at com.datastax.driver.core.querybuilder.QueryBuilderTest.batchCounterTest(QueryBuilderTest.java:455)

updateTest(com.datastax.driver.core.querybuilder.QueryBuilderTest)  Time elapsed: 0.004 sec  <<< FAILURE!
java.lang.AssertionError: expected [UPDATE foo.bar USING TIMESTAMP 42 SET a=12,b=[3,2,1],c=c+3 WHERE k=2;] but found [UPDATE foo.bar USING TIMESTAMP 42 SET a=12,b=[3,2,1],c=c+'00:00:00.000000003' WHERE k=2;]
    at org.testng.Assert.fail(Assert.java:94)
    at org.testng.Assert.failNotEquals(Assert.java:494)
    at org.testng.Assert.assertEquals(Assert.java:123)
    at org.testng.Assert.assertEquals(Assert.java:176)
    at org.testng.Assert.assertEquals(Assert.java:186)
    at com.datastax.driver.core.querybuilder.QueryBuilderTest.updateTest(QueryBuilderTest.java:275)

[olim7t]

CodecRegistry is not meant to be subclassed by clients

So should we make it final?

[adutra]

It is final already.

[olim7t]

protocolVersion doesn't seem to be used anywhere.

[adutra]

Indeed. I added that argument on purpose in anticipation of future use, and to be consistent with other methods in this class. But I'm fine if you prefer to remove it.