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

replace msgpack library #74

Merged
merged 10 commits into from
Jun 6, 2024
Merged

replace msgpack library #74

merged 10 commits into from
Jun 6, 2024

Conversation

nicolasparada
Copy link
Contributor

@nicolasparada nicolasparada commented May 24, 2024

Replace msgpack library from github.com/ugorji/go/codec to github.com/vmihailenco/msgpack/v5.

There are still references to the old library in the subppackages that I didn't want to touch yet.

It has better support for (un)marshalling types like time.Time making tests more expectables.

Also included tests from ##68 and msgpack unmarshalling into map[string]any instead of map[string]string.

Also did some chores and updated some dependencies, addressed some lint issues, etc.

@nicolasparada nicolasparada requested a review from a team as a code owner May 24, 2024 16:36
@nicolasparada
Copy link
Contributor Author

Notice that CI is failing, but it was failing from before. There are too many lint problems (maybe the linter is too strict considering we are using many C idioms).

Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

I confirmed that this change should not affect hot-reloading. And working as intended. But I'm still processing for how this replacement works.

Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

All confirmed for time related things. Looks attractive. 👍

@cosmo0920
Copy link
Contributor

Oh, I've overlooked for the requirements of this replacement. We need to handle timestamp related things to ordinary msgpack objects when it printed or ingested in the ordinary records.

@nicolasparada
Copy link
Contributor Author

Right. This does not fixes the issue that fluent-bit still renders Go's time.Time type as bytes.

Copy link

@jasmingacic jasmingacic left a comment

Choose a reason for hiding this comment

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

If I'm reading this correctly this is mostly refactoring to replace the library.
If that's the case it looks good to me

@nicolasparada nicolasparada merged commit 21668b6 into main Jun 6, 2024
8 of 9 checks passed
@nicolasparada nicolasparada deleted the record-time branch June 6, 2024 13:35
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