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

Allow to use headers in the deserializer object #534

Merged
merged 4 commits into from Jul 29, 2019

Conversation

@davydovanton
Copy link
Collaborator

@davydovanton davydovanton commented Jul 23, 2019

I played with 1.3 version and found an interesting case for improvement. Now we can use headers. I played with avro and you told me that it's good to put event type and version (for schema-registry) to the headers of kafka message.

And with it I'll be happy to use deserializer like this:

module Parsers
  class AvroParser
    REGESTRY_URL = 'http://localhost:8081/'

    attr_reader :avro

    def initialize
      @avro = AvroTurf::Messaging.new(registry_url: REGESTRY_URL)
    end

    def call(content, subject: '', version: 1)
      avro.decode(content, subject: subject, version: version)
    end
  end
end

In this case, I can decode the message by subject and version and get decoded payload for in consumer block. Also, it will provide me to use dry-struct for mapping payload to struct object, like this:

def call(content, subject: '', version: 1)
  STRUCTS[subject][version].new(
    avro.decode(content, subject: subject, version: version)
  )
end

class ArticlesSnapshotConsumer < ApplicationConsumer
  def consume
    params_batch.each do |message|
      message.payload # => Dry::Struct object
    end
  end
end

This PR is copy of #533. I just use karafka repository branch now for better maintainability.

@davydovanton
Copy link
Collaborator Author

@davydovanton davydovanton commented Jul 23, 2019

@mensfeld @paneq I updated a PR and also move the original branch to the karafka repository

Loading

@mensfeld
Copy link
Contributor

@mensfeld mensfeld commented Jul 24, 2019

Loading

@mensfeld mensfeld requested a review from nijikon Jul 24, 2019
@mensfeld
Copy link
Contributor

@mensfeld mensfeld commented Jul 24, 2019

Conceptually this is a really good idea and once we have it in master I will:

a) update the upgrade docs
b) update wiki
c) release 1.3.0rc1

Loading

@davydovanton
Copy link
Collaborator Author

@davydovanton davydovanton commented Jul 24, 2019

@mensfeld hey, I'll fix coditsu's issues today. Also, I can help with documentation if you want!

Loading

lib/karafka/serialization/json/deserializer.rb Outdated Show resolved Hide resolved
Loading
@davydovanton davydovanton force-pushed the feature/headers-in-deserialize branch from 6d943b0 to 4d2f3d0 Jul 24, 2019
@davydovanton davydovanton force-pushed the feature/headers-in-deserialize branch from 4d2f3d0 to ec0c1c4 Jul 24, 2019
@mensfeld
Copy link
Contributor

@mensfeld mensfeld commented Jul 25, 2019

Feel free to update: https://github.com/karafka/karafka/wiki/Serialization - we have this as a separate repo: https://github.com/karafka/wiki/ with which I sync this wiki, so please PR to the wiki one. Also if you have enough time, you could just double check that we don't describe serialization in any other place in the context of your work.

Loading

@mensfeld
Copy link
Contributor

@mensfeld mensfeld commented Jul 25, 2019

@davydovanton I'll merge it once we have wiki up2date not to confuse users.

Loading

@davydovanton
Copy link
Collaborator Author

@davydovanton davydovanton commented Jul 25, 2019

Loading

lib/karafka/params/params.rb Outdated Show resolved Hide resolved
Loading
@mensfeld
Copy link
Contributor

@mensfeld mensfeld commented Jul 29, 2019

@davydovanton added small remarks (see d7961cf). Once all green, we can merge!

Loading

@mensfeld mensfeld merged commit 03cc013 into master Jul 29, 2019
2 checks passed
Loading
@mensfeld mensfeld deleted the feature/headers-in-deserialize branch Jul 29, 2019
@davydovanton
Copy link
Collaborator Author

@davydovanton davydovanton commented Jul 29, 2019

thanks @mensfeld and @nijikon for review ❤️

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants