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 support #62

Merged
merged 29 commits into from
Apr 8, 2019
Merged

Add Avro support #62

merged 29 commits into from
Apr 8, 2019

Conversation

dannyd4315
Copy link
Contributor

@dannyd4315 dannyd4315 commented Mar 8, 2019

This PR implements the ability to encode messages in the Avro binary format prior to publishing to Kafka, this gives us the benefit reducing the size of our messages, and being able to use schema's for the messages we are producing.

By using a schema registry such as Confluent's Schema Registry producers can post the versioned schema to said registry and consumers are able to retrieve the correct version of the schema for the message its consuming.

I have utilised the great open-source avro-turf gem and also included the avro_patches gem to allow for logical types within schemas.

lib/streamy.rb Outdated Show resolved Hide resolved
@dannyd4315 dannyd4315 changed the title [WIP] Add Avro support Add Avro support Mar 27, 2019
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
lib/streamy/avro_event.rb Outdated Show resolved Hide resolved
lib/streamy/event.rb Show resolved Hide resolved
streamy.gemspec Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
streamy.gemspec Show resolved Hide resolved
lib/streamy/configuration.rb Show resolved Hide resolved
@balvig balvig force-pushed the dd/add-avro-support branch from ae8e7a3 to 3992d9f Compare March 29, 2019 01:41
@cookpad cookpad deleted a comment from houndci-bot Mar 29, 2019
@cookpad cookpad deleted a comment from houndci-bot Mar 29, 2019
@cookpad cookpad deleted a comment from houndci-bot Mar 29, 2019
@cookpad cookpad deleted a comment from houndci-bot Mar 29, 2019
@cookpad cookpad deleted a comment from houndci-bot Mar 29, 2019
@cookpad cookpad deleted a comment from houndci-bot Mar 29, 2019
@cookpad cookpad deleted a comment from houndci-bot Mar 29, 2019
Copy link
Contributor

@balvig balvig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it a little difficult to do a proper suggestions branch, as what I wanted to show relied on the refactoring in #63, hope it's ok that I pushed the commits directly here instead and that they make sense 🙇

Mainly I didn't feel like we needed to repeat all those "method not implemented" tests for both classes, but let me know if that seems wrong.

Otherwise looks 👍 to me, this avro thing looks really useful, nice!

streamy.gemspec Show resolved Hide resolved
Copy link
Contributor

@ChrisBr ChrisBr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 👍

@dannyd4315
Copy link
Contributor Author

♻️ Please - Updated rspec helper that is used by global-web

Copy link
Contributor

@balvig balvig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 but I think there is one remaining issue we need to sort out:

Basically we include two types of test helpers:

assert_published_event # for minitest, test-unit
expect_event # for RSpec

...and I guess it's sort of expected that they behave the same way 🤔

Currently, only the RSpec one "hashifys" the deliveries hash?

Indeed, although it is currently "dormant" 😴, global-reports makes use of assert_published_event:

    assert_published_event(
      topic: "pageviews",
      type: "counted_recipe_views",
      body: hash_including(
        from: Time.zone.parse("2017/01/01"),
        to: Time.zone.parse("2017/01/02"),
        counts: counts
      )
    )

Source

...and I suppose it would be subject to the same problem as global-web.

Meanwhile, within this lib, in the tests we've written I guess we're finding it useful to test the "raw" attributes that get sent in some situations, to check that things are actually working.

I think it would be fine to merge and move on to make progress, but moving forward maybe you could try to find a way to handle both of these use cases, and make sure the helpers stay consistent?

@dannyd4315
Copy link
Contributor Author

dannyd4315 commented Apr 8, 2019

I think it would be fine to merge and move on to make progress, but moving forward maybe you could try to find a way to handle both of these use cases, and make sure the helpers stay consistent?

👍 Thanks, will do. Will take a look at that next. Version bumping to 0.3.0 to incorporate these changes.

@dannyd4315 dannyd4315 merged commit 244ccb8 into master Apr 8, 2019
@dannyd4315 dannyd4315 deleted the dd/add-avro-support branch April 8, 2019 08:40
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.

5 participants