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 JsonFormatStreamReader #878

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

Conversation

iosifnicolae2
Copy link

No description provided.

@iosifnicolae2
Copy link
Author

Fixes #874
Partially fixes #869

@fqlx
Copy link

fqlx commented Dec 3, 2022

@andmarios Could someone your team review this? I need this change as well to backup everything from kafka (timestamp, key, value, headers, and message)

Copy link
Collaborator

@davidsloan davidsloan left a comment

Choose a reason for hiding this comment

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

Sorry for taking a while to look at this one.

A few comments:

  • Please remove "CONTRIBUTING" file - we will be adding something similar soon. Also not appropriate for us to have any mention of other companies.
  • Addition of Dockerfile looks good but please can you document how this is used or how this can help development in the README.
  • I don't think the MANIFEST.MF is necessary as this should be generated by the assembly process? Is there a reason you needed to add this?
  • Any code addition should be covered with unit tests so as to ensure the code is bug-free and cover against future regressions. We plan on covering this in some contribution guidelines in future, apologies for not making this clear.

@davidsloan
Copy link
Collaborator

I forgot to mention, thank you for the contribution @iosifnicolae2 👍

@iosifnicolae2
Copy link
Author

@davidsloan thank you for your review, unfortunetly we're not using anymore this functionality so I cannot allocate more time on fixing all the requested changes.

@fqlx if you have time feel free to fix the requested changes and open a new PR

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants