#46 - Implement KafkaSerializer for JSON serialization instead using … #49
Conversation
…using explicit object mapping
|
Event and AdminEvent does not have a common parent class. Because of this Object is needed. |
| } | ||
|
|
||
| @Override | ||
| public byte[] serialize(String topic, Object data) { |
There was a problem hiding this comment.
What's the difference to org.apache.kafka.common.serialization.BytesSerializer?
There was a problem hiding this comment.
It throws exception on Events and AdminEvents
| Map<String, Object> optionalProperties) { | ||
| Properties props = new Properties(); | ||
| props.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, bootstrapServer); | ||
| props.put(ProducerConfig.CLIENT_ID_CONFIG, clientId); | ||
| props.put(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); | ||
| props.put(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); | ||
| props.put(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, JsonSerializer.class.getName()); |
There was a problem hiding this comment.
This will be a breaking change for all system that uses plain json strings. I think it would be much better to make this configurable and rely and on the default serializer from the org.apache.kafka.common.serialization package.
There was a problem hiding this comment.
Yes you are right, it was a mistake to change the KEY_SERIALIZER. I fixed it.
In my next PR both will be configurable. Thank you!
|
@SnuK87 So I managed to solve the issues but it required huge modifications on the code. Please feel free to decide if you want to merge it or not. All of your unit tests passes. I think with this AVRO improvement this SPI could be used in large scale projects with confidence. Thank you! |
|
This change would destroy the whole purpose of this library. It should be as generic as possible, can be easily used with standard kafka and keycloak setups and is easy to customize for your specific use case (which is your case I guess). Here are some things that would make this library useless for other people:
|
|
It's ok if you don't merge this one. I understand.
|
No description provided.