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 protobuf deserialization to consumer and serialization to producer #111

Merged
merged 1 commit into from Dec 3, 2021

Conversation

xakep666
Copy link
Contributor

@xakep666 xakep666 commented Dec 1, 2021

Description

Added protobuf serialization in producer and deserialization in consumer.
To use this user must supply raw .proto schemas by using --proto-file flag and --proto-import-path flag to resolve dependencies mentioned in import sections. User also can supply compiled .protoset file(s) using --protoset-file flag.
Values for flags above also duplicated in config file.

To enable protobuf serialization or deserialization user must specify --value-proto-message flag which contains message type name.

Key may be optionally serialized/deserialized by using flag --key-proto-message.

Fixes #110

Type of change

  • New feature (non-breaking change which adds functionality)

Documentation

  • the change is mentioned in the ## [Unreleased] section of CHANGELOG.md
  • the configuration yaml was changed and the example config in README.md was updated
  • a usage example was added to README.md

Copy link
Collaborator

@d-rk d-rk left a comment

Choose a reason for hiding this comment

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

Good job @xakep666 👍

I reviewed your PR and the majority of my comments are just cosmetic changes.
The only needed change to the feature itself, would be adding a caching mechanism for retrieving the protobuf descriptors.

cmd/consume/consume.go Outdated Show resolved Hide resolved
cmd/produce/produce.go Outdated Show resolved Hide resolved
internal/consumer/ChainMessageDeserializer.go Outdated Show resolved Hide resolved
internal/consumer/ChainMessageDeserializer.go Outdated Show resolved Hide resolved
internal/consumer/ProtobufMessageDeserializer.go Outdated Show resolved Hide resolved
internal/consumer/ProtobufMessageDeserializer.go Outdated Show resolved Hide resolved
cmd/consume/consume_test.go Outdated Show resolved Hide resolved
cmd/consume/consume_test.go Outdated Show resolved Hide resolved
internal/helpers/protobuf.go Outdated Show resolved Hide resolved
internal/helpers/protobuf.go Outdated Show resolved Hide resolved
@xakep666
Copy link
Contributor Author

xakep666 commented Dec 2, 2021

@random-dwi what about linter error (deprecated github.com/golang/protobuf/jsonpb). Is it ok to add exclusion comment or rule for this?

@d-rk
Copy link
Collaborator

d-rk commented Dec 2, 2021

@xakep666 regarding the linter error, I would prefer not to introduce already deprecated stuff with a PR.
the linter error suggests to replace it by google.golang.org/protobuf/encoding/protojson. Does that require more refactoring then just replacing some parts?

@xakep666
Copy link
Contributor Author

xakep666 commented Dec 2, 2021

@xakep666 regarding the linter error, I would prefer not to introduce already deprecated stuff with a PR. the linter error suggests to replace it by google.golang.org/protobuf/encoding/protojson. Does that require more refactoring then just replacing some parts?

@random-dwi This is dependency of protoreflect and there is issue about it jhump/protoreflect#463 so simple replacement is not possible

@xakep666 xakep666 requested a review from d-rk December 2, 2021 16:25
@d-rk d-rk merged commit a02978e into deviceinsight:master Dec 3, 2021
@d-rk
Copy link
Collaborator

d-rk commented Dec 3, 2021

@xakep666 I just released version 1.24.0 with the feature included 🎉

@xakep666 xakep666 deleted the feature/protobuf branch December 3, 2021 13:30
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.

Protobuf support
2 participants