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

parser_json: fix wrong LoadError warning #4522

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Jun 6, 2024

Which issue(s) this PR fixes:
None

What this PR does / why we need it:
If Oj is not installed, LoadError with the empty message is raised.

So, the current condition /\boj\z/.match?(ex.message) does not work and the following meaningless warning is displayed.

{datetime} [warn]: #x {id} LoadError

After this fix, the log message will be:

{datetime} [info]: #x {id} Oj is not installed, and failing back to Yajl for json parser

Docs Changes:
Not needed.

Release Note:
Same as the title.

@daipom daipom added this to the v1.16.6 milestone Jun 6, 2024
@daipom daipom force-pushed the parser_json-fix-wrong-LoadError-warning branch from b25e989 to 151e631 Compare June 7, 2024 08:21
Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

It seems that this rescue no longer needed.
It would be better to do retry at when :oj instead of rescue block.

@daipom daipom force-pushed the parser_json-fix-wrong-LoadError-warning branch 2 times, most recently from d515b3a to 29ff381 Compare June 12, 2024 02:53
@daipom
Copy link
Contributor Author

daipom commented Jun 12, 2024

It seems that this rescue no longer needed. It would be better to do retry at when :oj instead of rescue block.

Thanks for your review!
I have fixed this as https://github.com/fluent/fluentd/compare/151e6314a371add407f602d50cb8be777109afd1..d515b3a6a277d854378a5961223b4666fb438c31

If Oj is not installed, LoadError with the empty message is raised.
So, the current condition `/\boj\z/.match?(ex.message)` does not work
and the following meaningless warning is displayed.

    {datetime} [warn]: #x {id} LoadError

After this fix, the log message will be:

    {datetime} [info]: #x {id} Oj is not installed, and failing back to
    Yajl for json parser

Refactor "rescue" logic because this falling back feature is
currently only for "oj" (LoadError can not occur for "json" and
"yajl").

Signed-off-by: Daijiro Fukuda <[email protected]>
Co-authored-by: Takuro Ashie <[email protected]>
@daipom daipom force-pushed the parser_json-fix-wrong-LoadError-warning branch from 29ff381 to 891ce71 Compare June 12, 2024 02:56
@daipom
Copy link
Contributor Author

daipom commented Jun 12, 2024

It looks like failing back should be falling back.
For now, this PR does not handle it.
(Changing log messages should not be done casually. For this level, it might be better to leave the existing.)

@daipom daipom requested a review from ashie June 12, 2024 03:01
@ashie ashie merged commit c0cd1e6 into fluent:master Jun 12, 2024
16 checks passed
@ashie
Copy link
Member

ashie commented Jun 12, 2024

Thanks!

@daipom daipom deleted the parser_json-fix-wrong-LoadError-warning branch June 12, 2024 06:31
@daipom
Copy link
Contributor Author

daipom commented Jun 12, 2024

Thanks for your review!

@daipom daipom modified the milestones: v1.16.6, v1.17.1 Aug 15, 2024
@daipom daipom added the backport to LTS We will backport this fix to the LTS branch label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport to LTS We will backport this fix to the LTS branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants