Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Avro message deserialization capability #33

Merged
merged 15 commits into from
Dec 19, 2018

Conversation

lcary
Copy link
Contributor

@lcary lcary commented Nov 14, 2018

This pull request has the following features:

  1. Adds a new dropdown to the message viewer screen, which allows users to select a message format (e.g. "AVRO").
  2. Adds an AvroMessageDeserializer class to display Avro-serialized messages, which are indented for readability using GSON.
  3. Allows user to configure Schema Registry URL via configuration settings (optional unless Avro message viewing is being used).
  4. Allows user to configure an optional, default message format via configuration settings.
  5. Bumps the library dependency on kafka-clients to version 0.10.2.2.

This change has been tested for viewing both normal and Avro-serialized Kafka messages, and should be backward-compatible with the existing Kafdrop use cases.

See the below screenshot for an example of the new "Message Format" dropdown and Avro-deserialized messages:

screen shot 2018-12-12 at 11 05 17 pm

See the README changes for new configuration settings in CLI form.

@lcary
Copy link
Contributor Author

lcary commented Nov 16, 2018

Hello @dhayha, what's required to get this pull request merged in? I'm trying to upstream our changes so that we don't have to maintain a fork. It should be compatible with the existing use cases, since the schema registry / avro deserialization capability is controlled via an optional flag. I can make any style/test/etc adjustments, just say the word.

@dhayha
Copy link
Contributor

dhayha commented Nov 27, 2018

Sorry for taking so long to get back to you. We definitely appreciate the contribution!

The main concern I have is that the MessageInspector now has to know whether the message uses Avro or JSON. One way to abstract this concept would be to define a MessageDeserializer interface that could be used by the MessageInspector to deserialize messages using whatever format necessary. If this could be auto-detected, great! However my instinct says that we'd need some kind of control on the UI (e.g. radio buttons, drop down) to allow the user to select what type of message format to use (maybe with a configurable default). Then, based on the user's choice, instead of passing in a schema URL, the controller passes in a MessageDeserializer instance

@lcary
Copy link
Contributor Author

lcary commented Nov 27, 2018 via email

* Move deserialization logic into MessageDeserializer classes

* Use enums to select deserializer type in message inspector view

* Clean up MessageController to remove comments and unnecessary decorators
@lcary
Copy link
Contributor Author

lcary commented Dec 13, 2018

@dhayha the changes you suggested have been incorporated into this pull request (see: 4c079ed). Could you please take another look when you have some time?

* Refactor deserializers to utils directory and add default message format config

* Update readme for new message format config setting
@dhayha dhayha merged commit 16eca46 into HomeAdvisor:master Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants