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.

Sign up for GitHub

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

discards packets which are not (fully) populated #3508

Merged
merged 1 commit into from
Nov 7, 2024
Merged

discards packets which are not (fully) populated #3508

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions perf/src/packet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,11 @@ impl PacketBatch {
// break the payload into smaller messages, and here any errors
// should be propagated.
error!("Couldn't write to packet {:?}. Data skipped.", e);
packet.meta_mut().set_discard(true);
Copy link

Choose a reason for hiding this comment

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

Idea makes sense, what do you think about moving line inside Packet::populate_packet so that we get this behavior across the board, not just from this one PacketBatch constructor ?

Copy link
Author

Choose a reason for hiding this comment

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

I would rather not do that.
If there is a failure, Packet::populate_packet is returning the error to the caller. The caller may choose to either:

  1. discard the packet (like in this commit).
  2. or, reuse the packet and write the next payload to it. In which case it shouldn't be marked discard.

The problem with the current code is that the caller is effectively ignoring the error.

Copy link

Choose a reason for hiding this comment

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

or, reuse the packet and write the next payload to it. In which case it shouldn't be marked discard.

Sure, but if the caller is making a conscious decision to "recycle" the packet, it doesn't seem unreasonable to call .set_discard(false) to clean up this "dirtied" packet

Copy link
Author

Choose a reason for hiding this comment

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

That would require the caller to know the implementation details Packet::populate_packet and what side effects it causes when it fails, which is generally not good.

The error returned by Packet::populate_packet however is part of the signature and explicit.

}
} else {
trace!("Dropping packet, as destination is unknown");
packet.meta_mut().set_discard(true);
}
}
batch
Expand Down
Loading