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

input_chunk: info level instead of debug for chunk removal msg #6719

Merged
merged 1 commit into from
Jan 28, 2023

Conversation

PettitWesley
Copy link
Contributor

Signed-off-by: Wesley Pettit [email protected]


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@PettitWesley PettitWesley temporarily deployed to pr January 20, 2023 22:37 — with GitHub Actions Inactive
@PettitWesley PettitWesley temporarily deployed to pr January 20, 2023 22:37 — with GitHub Actions Inactive
flb_debug("[input chunk] remove route of chunk %s with size %ld bytes to output plugin %s "
"to place the incoming data with size %ld bytes", flb_input_chunk_get_name(old_ic),
old_ic_bytes, o_ins->name, chunk_size);
flb_info("[input chunk] remove route of chunk %s with size %zd bytes to output plugin %s "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the format specifiers here were wrong according to my understanding.

old_ic_bytes is ssize_t which should use %zd for signed type, and then the others are size_t which is unsigned and needs %zu

Copy link
Contributor Author

@PettitWesley PettitWesley Jan 20, 2023

Choose a reason for hiding this comment

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

In terms of changing from debug => info, my thinking was that the user should get a clear message when chunks are being deleted.

Copy link
Member

Choose a reason for hiding this comment

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

if I am not wrong the debug message is a "hint" to tell the user/debugger the chunk "aims to be removed", actually that routine is trying to find a candidate since the queue is full, then the logic depends on routes/mask, at that level of the message is not really removed.

cc: @leonardo-albertovich @pwhelan -> I think you both were involved in those changes, can you comment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, in that case then I think the debug message on line 624 should be info and should be updated since that is where the deletion truly happens? And then here the debug message should say something like "Attempt to remove".

Copy link
Contributor

Choose a reason for hiding this comment

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

flb_task_destroy also destroys the chunk backing the task. You would need to change the call to flb_debug on both lines 616 and 624.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack. I will update to 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.

@pwhelan @edsiper I updated the existing debug message to be "consider removal" and then made a nice new info message in both places where we actually delete a chunk. Please re-review.

@PettitWesley PettitWesley temporarily deployed to pr January 20, 2023 22:58 — with GitHub Actions Inactive
@PettitWesley PettitWesley temporarily deployed to pr January 26, 2023 23:17 — with GitHub Actions Inactive
@PettitWesley PettitWesley temporarily deployed to pr January 26, 2023 23:17 — with GitHub Actions Inactive
@PettitWesley PettitWesley requested a review from pwhelan January 26, 2023 23:17
@PettitWesley PettitWesley temporarily deployed to pr January 26, 2023 23:35 — with GitHub Actions Inactive
Copy link
Contributor

@pwhelan pwhelan left a comment

Choose a reason for hiding this comment

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

LGTM

@edsiper edsiper merged commit b725d6b into fluent:master Jan 28, 2023
sumitd2 pushed a commit to sumitd2/fluent-bit that referenced this pull request Feb 8, 2023
PettitWesley added a commit to PettitWesley/fluent-bit that referenced this pull request May 2, 2023
PettitWesley added a commit to PettitWesley/fluent-bit that referenced this pull request Jun 2, 2023
PettitWesley added a commit to PettitWesley/fluent-bit that referenced this pull request Jun 8, 2023
matthewfala pushed a commit to matthewfala/fluent-bit that referenced this pull request Sep 23, 2023
PettitWesley added a commit to PettitWesley/fluent-bit that referenced this pull request May 22, 2024
zhihonl pushed a commit to zhihonl/fluent-bit that referenced this pull request Aug 20, 2024
swapneils pushed a commit to amazon-contributing/upstream-to-fluent-bit that referenced this pull request Oct 3, 2024
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