KAFKA-4769: Add Float serializer, deserializer, serde #2554

miguno · 2017-02-16T05:54:29Z

No description provided.

miguno · 2017-02-16T05:55:04Z

Please review any of @guozhangwang @enothereska @dguy @mjsax

miguno · 2017-02-16T05:57:36Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

+        deserializer.close();
+    }
+
+    @Test(expected = SerializationException.class)


These exception-related tests are missing for most of the existing serdes, but I decided to add them after inspecting the results of the code coverage report.

Then, would it better to add exception-related tests to the other SerDes also? How do you think?

IMHO out of scope for this PR, which is adding Float support.

Totally agree. It would be much better for me to do the following work.

asfbot · 2017-02-16T06:35:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1702/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-02-16T06:40:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1705/
Test PASSed (JDK 8 and Scala 2.11).

dongjinleekr · 2017-02-16T06:53:41Z

clients/src/main/java/org/apache/kafka/common/serialization/Serdes.java

@@ -111,6 +117,10 @@ public ByteArraySerde() {
            return (Serde<T>) Long();
        }

+        if (Float.class.isAssignableFrom(type)) {


I have a question: Would type instanceof Float be better? Or, you decided to use Class#isAssignableFrom method for consistency only?

Consistency.

Also, note that type is a Class<T> -- a check with instanceof would not work because it requires an instance of a class, whereas serdeFrom(Class<T> type) means we must work with a class object.

Aha! Understood. Thanks for your answer! 😄

asfbot · 2017-02-16T07:09:25Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1702/
Test FAILed (JDK 8 and Scala 2.12).

enothereska · 2017-02-16T10:54:18Z

LGTM

dguy · 2017-02-16T16:56:52Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

@@ -119,6 +120,49 @@ public void testLongSerializer() {
    }

    @Test
+    public void testFloatSerializer() {


shouldSerializeDeserializeFloat

dguy · 2017-02-16T16:57:13Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

@@ -119,6 +120,49 @@ public void testLongSerializer() {
    }

    @Test
+    public void testFloatSerializer() {
+        Float[] floats = new Float[]{


nit: please use final everywhere

dguy · 2017-02-16T16:58:40Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

+        Deserializer<Float> deserializer = Serdes.Float().deserializer();
+
+        for (Float value : floats) {
+            assertEquals("Should get the original double after serialization and deserialization",


i'd prefer we use asserThat, i.e., assertThat(value, equalTo(deserializer.deserialize(...))

Done (didn't know it was ok to use it, the existing tests didn't)

dguy · 2017-02-16T16:59:13Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

+                    value, deserializer.deserialize(topic, serializer.serialize(topic, value)));
+        }
+
+        assertEquals("Should support null in serialization and deserialization",


make this into a different test, i.e., shouldSupportNullInFloatSerde or similiar

dguy · 2017-02-16T17:00:26Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

+        assertEquals("Should support null in serialization and deserialization",
+                null, deserializer.deserialize(topic, serializer.serialize(topic, null)));
+
+        serializer.close();


these calls aren't needed, right? close does nothing anyway. And if they did do something, then you'd probably want to do it in tearDown rather than here.

these calls aren't needed, right? close does nothing anyway.

Yes, close doesn't do anything in this specific case, but that behavior is not part of the contract afaict. Hence IMHO we should always close it.

And if they did do something, then you'd probably want to do it in tearDown rather than here.

That would only work if we created a global serde/serializer/deserializer instance that we'd re-use between tests. It's a matter of personal preference I admit, but I'd rather avoid doing that (I'm ok with the re-usable topic variable though). That said, I don't have a strong opinion. If we do change the pattern, then the existing tests should be updated, too (though outside of this PR) for the sake of consistency.

dguy · 2017-02-16T17:02:28Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

+        deserializer.close();
+    }
+
+    @Test(expected = SerializationException.class)


We have a rule w.r.t using @expected the rule is that it should only be used for single line tests where you can guarantee that the exception can only come from the line that is being executed. In all other cases you should use try{...}catch(...)

dguy · 2017-02-16T19:56:11Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

+    @Test
+    public void floatSerializerShouldReturnNullForNull() {
+        final Serializer<Float> serializer = Serdes.Float().serializer();
+        assertThat(null, equalTo(serializer.serialize(topic, null)));


this could be: assertThat(serializer.serialize(topic, null), nullValue())

dguy · 2017-02-16T19:56:20Z

clients/src/test/java/org/apache/kafka/common/serialization/SerializationTest.java

+    @Test
+    public void floatDeserializerShouldReturnNullForNull() {
+        final Deserializer<Float> deserializer = Serdes.Float().deserializer();
+        assertThat(null, equalTo(deserializer.deserialize(topic, null)));


this one, too

dguy · 2017-02-16T19:56:59Z

clients/src/main/java/org/apache/kafka/common/serialization/FloatSerializer.java

+    }
+
+    @Override
+    public byte[] serialize(String topic, Float data) {


nit: (Sorry should've mentioned previously) would you mind making the params final, too?

asfbot · 2017-02-16T19:59:46Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1717/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-02-16T20:02:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1714/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-02-16T20:06:56Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1714/
Test PASSed (JDK 7 and Scala 2.10).

dguy

LGTM

asfbot · 2017-02-16T21:50:17Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1720/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-02-16T21:51:30Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1717/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-02-16T21:52:50Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1717/
Test PASSed (JDK 8 and Scala 2.12).

miguno · 2017-02-16T21:58:08Z

FYI: I squashed the commits. Ping @guozhangwang for final review.

asfbot · 2017-02-16T22:01:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1721/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-02-16T22:44:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1724/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-02-16T22:45:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1721/
Test PASSed (JDK 8 and Scala 2.12).

cmccabe · 2017-02-17T22:13:37Z

clients/src/main/java/org/apache/kafka/common/serialization/FloatSerializer.java

+        if (data == null)
+            return null;
+
+        long bits = Float.floatToIntBits(data);


Why are we using floatToIntBits rather than floatToRawIntBits?

@cmccabe No strong reason either way. AFAIK there's no specific contract or requirement for handling NaN. Thoughts?

asfbot · 2017-02-28T00:31:06Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1873/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-02-28T00:31:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1871/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-02-28T00:32:14Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1870/
Test PASSed (JDK 7 and Scala 2.10).

mjsax

LGTM

guozhangwang · 2017-03-13T18:50:15Z

LGTM and merged to trunk.

miguno commented Feb 16, 2017

View reviewed changes

dongjinleekr reviewed Feb 16, 2017

View reviewed changes

asfgit force-pushed the trunk branch from 5fcc458 to a1900ad Compare February 16, 2017 14:40

dguy reviewed Feb 16, 2017

View reviewed changes

dguy approved these changes Feb 16, 2017

View reviewed changes

KAFKA-4769: Add Float serializer, deserializer, serde

6b714d6

cmccabe reviewed Feb 17, 2017

View reviewed changes

Use floatToRawIntBits rather than floatToIntBits

23e7452

mjsax approved these changes Mar 13, 2017

View reviewed changes

asfgit closed this in cd69daa Mar 13, 2017

KAFKA-4769: Add Float serializer, deserializer, serde #2554

KAFKA-4769: Add Float serializer, deserializer, serde #2554

Conversation

miguno commented Feb 16, 2017

miguno commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Feb 16, 2017

asfbot commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguno Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Feb 16, 2017

enothereska commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguno Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Feb 16, 2017

asfbot commented Feb 16, 2017

asfbot commented Feb 16, 2017

dguy left a comment

Choose a reason for hiding this comment

asfbot commented Feb 16, 2017

asfbot commented Feb 16, 2017

asfbot commented Feb 16, 2017

miguno commented Feb 16, 2017

asfbot commented Feb 16, 2017

asfbot commented Feb 16, 2017

asfbot commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Feb 28, 2017

asfbot commented Feb 28, 2017

asfbot commented Feb 28, 2017

mjsax left a comment

Choose a reason for hiding this comment

guozhangwang commented Mar 13, 2017

miguno Feb 16, 2017 •

edited

Loading

miguno Feb 16, 2017 •

edited

Loading