Kafka
  1. Kafka
  2. KAFKA-1049

Encoder implementations are required to provide an undocumented constructor.

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      So, it seems that if I want to set a custom serializer class on the producer (in 0.8), I have to use a class that includes a special constructor like:

      public class MyKafkaEncoder<MyType> implements Encoder<MyType> {
      // This constructor is expected by the kafka producer, used by reflection
      public MyKafkaEncoder(VerifiableProperties props)

      { // what can I do with this? }

      @Override
      public byte[] toBytes(MyType message)

      { return message.toByteArray(); }

      }

      It seems odd that this would be a requirement when implementing an interface. This seems not to have been the case in 0.7.

      What could my encoder class do with the VerifiableProperties?

        Activity

        Hide
        Jason Rosenberg added a comment -

        I posed this issue to the kafka users mailing list. There were a couple suggestions:

        Jay Kreps suggests that the producer when instantiating an encoder, could first try to use a constructor that takes a VerifiableProperties, and if that is not available, then revert to a simple no-arg constructor.

        Joel Koshy suggests that VerifiableProperties could be made to extend Properties, and that might be an easier thing to require for the java api.

        My personal feeling, is that it's hard to require implementers of an interface to provide a constructor of a certain signature, since there's no way to enforce this statically at compile time. Perhaps instead, the interface can include a 'setProperties()' method, that the producer will call right after instantiation.

        Show
        Jason Rosenberg added a comment - I posed this issue to the kafka users mailing list. There were a couple suggestions: Jay Kreps suggests that the producer when instantiating an encoder, could first try to use a constructor that takes a VerifiableProperties, and if that is not available, then revert to a simple no-arg constructor. Joel Koshy suggests that VerifiableProperties could be made to extend Properties, and that might be an easier thing to require for the java api. My personal feeling, is that it's hard to require implementers of an interface to provide a constructor of a certain signature, since there's no way to enforce this statically at compile time. Perhaps instead, the interface can include a 'setProperties()' method, that the producer will call right after instantiation.
        Hide
        Jay Kreps added a comment -

        I agree that an init() method avoids the non-type-checked contructor-type requirement, but since serializers are used in a multithreaded context this creates difficulties since all the member variables are now mutable and need to be synchronized.

        Show
        Jay Kreps added a comment - I agree that an init() method avoids the non-type-checked contructor-type requirement, but since serializers are used in a multithreaded context this creates difficulties since all the member variables are now mutable and need to be synchronized.
        Hide
        Jason Rosenberg added a comment -

        Perhaps it would be cleaner to have the api specify a factory class instead of the actual encoder class. This way, the factory can implement an api which requires a getEncoder(Properties props) method.

        Show
        Jason Rosenberg added a comment - Perhaps it would be cleaner to have the api specify a factory class instead of the actual encoder class. This way, the factory can implement an api which requires a getEncoder(Properties props) method.
        Hide
        Joel Koshy added a comment -
        Show
        Joel Koshy added a comment - Created reviewboard https://reviews.apache.org/r/14188/
        Hide
        Sharmarke Aden added a comment - - edited

        What exactly is the value of VerifiableProperties? Why would you want to implicitly drive implementation details of Encoder/Decoder interfaces? If you really need to make VerifiableProperties available to encoder/decoder implementations then pass it in via toBytes/fromBytes methods.

        This issue is a major API flaw. Please fix it as soon as possible.

        public interface Encoder<T> {
            public byte[] toBytes(T object, VerifiableProperties properties);
        }
        
        public interface Decoder<T> {
            public T fromBytes(byte[] buffer, VerifiableProperties properties);
        }
        
        Show
        Sharmarke Aden added a comment - - edited What exactly is the value of VerifiableProperties? Why would you want to implicitly drive implementation details of Encoder/Decoder interfaces? If you really need to make VerifiableProperties available to encoder/decoder implementations then pass it in via toBytes/fromBytes methods. This issue is a major API flaw. Please fix it as soon as possible. public interface Encoder<T> { public byte [] toBytes(T object, VerifiableProperties properties); } public interface Decoder<T> { public T fromBytes( byte [] buffer, VerifiableProperties properties); }
        Hide
        Jay Kreps added a comment -

        The purpose is to allow configuration of the serializer. For example a string serializer needs a charset. An avro serializer needs a place to fetch avro schemas from. Etc.

        We have been working on redoing the producer and in the new producer the serialization is done by the user with the producer just taking the serialized bytes so that should make this somewhat more straight-forward.

        Show
        Jay Kreps added a comment - The purpose is to allow configuration of the serializer. For example a string serializer needs a charset. An avro serializer needs a place to fetch avro schemas from. Etc. We have been working on redoing the producer and in the new producer the serialization is done by the user with the producer just taking the serialized bytes so that should make this somewhat more straight-forward.
        Hide
        Sharmarke Aden added a comment -

        Is it safe to assume that going forward the onus of encoder/decoder configuration will be on the implementing class and that VerifiableProperties will not be required by Encoder/Decoder implementations? Or are you saying that Kafka will no longer provide/deprecate Encoder/Decoder interfaces and default StringEncoder/StringDecoder classes?

        +1 for getting rid of Encoder/Decoder classes and having producer/consumer deal strictly in bytes.

        Show
        Sharmarke Aden added a comment - Is it safe to assume that going forward the onus of encoder/decoder configuration will be on the implementing class and that VerifiableProperties will not be required by Encoder/Decoder implementations? Or are you saying that Kafka will no longer provide/deprecate Encoder/Decoder interfaces and default StringEncoder/StringDecoder classes? +1 for getting rid of Encoder/Decoder classes and having producer/consumer deal strictly in bytes.
        Hide
        Jay Kreps added a comment -

        Yeah, the plan was just to accept byte[] for key and value so the configuration would be in the client code. Discussion here: http://bit.ly/1l6ZPjl

        Show
        Jay Kreps added a comment - Yeah, the plan was just to accept byte[] for key and value so the configuration would be in the client code. Discussion here: http://bit.ly/1l6ZPjl
        Hide
        Sharmarke Aden added a comment -

        Very very Nice! Is it too late to chime in on this new API?

        Show
        Sharmarke Aden added a comment - Very very Nice! Is it too late to chime in on this new API?
        Hide
        Jay Kreps added a comment -

        No definitely not.

        Show
        Jay Kreps added a comment - No definitely not.
        Hide
        Jay Kreps added a comment -

        Obsolete now that we have the new clients.

        Show
        Jay Kreps added a comment - Obsolete now that we have the new clients.

          People

          • Assignee:
            Unassigned
            Reporter:
            Jason Rosenberg
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development