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

Encoder interface #194

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

fabiojmendes
Copy link
Contributor

This is pretty much WIP but I think we can start the discussion.

On my TODO I have:

  • add some integration tests for both proto and avro (avro is a little bit trickier because gnomock doesn't seem to expose the schema-registry port)
  • document the interface
  • use it for the consumer as well

}

func keyEncoder() codec.Encoder {
if keyProtoType != "" {
Copy link
Owner

@birdayz birdayz Apr 29, 2022

Choose a reason for hiding this comment

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

this might be the time for a new flag edit: (it's actually not a breaking change)

We could add:
--key-codec auto|proto|avro-json|avro

auto would be default, and behave as-is. it would try its best to decide what do do: if proto key/value flags are given, it will try proto. if avro flags are given, it will try avro-json. but it can be set to specifically pick avro (see #195)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this one, for proto I think it would be a little redundant because you have to specify --key-proto-type anyways. I guess we don't have the same problem we have with avro here.

Maybe a flag specific for avro would be enough. It also doesn't need to be specific to keys, It can apply for both keys and values at the same time. Something like:

value | kaf produce --avro-key-schema-id x -- avro-schema-id y --key 'key' --avro-input [json|avro-json]

What do you think?

There's a separate issue with avro and proto though. The current behaviour is a little misleading imo. If you specify both proto and avro, proto takes precedence. Maybe we should have some validation and throw an error in this case.

Copy link
Owner

Choose a reason for hiding this comment

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

it's ok for me. do you know if "avro-json" is the right term? is there any "better" term in the avro world for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, I'll check their docs see if I find anything. Another option would be just a boolean flag like --avro-strict then the docs can say something regarding being strict about unions.

A wiki page with examples of what we've been talking would be nice too.

pkg/avro/avro.go Outdated Show resolved Hide resolved
pkg/codec/codec.go Outdated Show resolved Hide resolved
test/user.avro Outdated Show resolved Hide resolved
@fabiojmendes
Copy link
Contributor Author

Speaking of tests, I faced a very weird issue when trying to add another produce/consume test.

Take a look on this commit, pretty straight forward. But the tests fails.

If I remove the timeout context it will fix it, but it is still weird.

It looks like the cleanup between tests runs is not working properly. Have you faced this as well?

@birdayz
Copy link
Owner

birdayz commented Apr 29, 2022

Speaking of tests, I faced a very weird issue when trying to add another produce/consume test.

Take a look on this commit, pretty straight forward. But the tests fails.

If I remove the timeout context it will fix it, but it is still weird.

It looks like the cleanup between tests runs is not working properly. Have you faced this as well?

can't say too much about this, sorry

@fabiojmendes fabiojmendes marked this pull request as ready for review May 2, 2022 22:23
@rakesh-tmdc
Copy link

Hi Team, Kindly resolve this conflict as this is causing production level issues in benthos plugin. @fabiojmendes Thanks

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.

3 participants