[Fix CQL Sticher 4/4] Move StreamID assignment to ParseFramesLoop #1732
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JamesMBartlett
pushed a commit
that referenced
this pull request
Nov 2, 2023
#1716) Summary: Modifies all protocol parsers to use a map of streams to deques by default. Protocols which do not have a notion of streams are encoded as single keys in a map. This completes the CQL stitcher fix and should simplify stitching frames for protocols with streams. The final PR in this sequence #1732 populates a map of streamIDs to deque of frames in ParseFramesLoop instead of ParseFrames. This should provide a small efficiency boost, as we won't have to loop over the frames twice. Related issues: Closes #1375 Type of change: /kind bug Test Plan: Updated parsing tests to use new interface. `Note`: this PR relies on changes introduced in #1689 and #1715 --------- Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
benkilimnik
force-pushed
the
fix-cql-stitcher-4
branch
from
05b8418
to
02d64f6
Compare
etep
reviewed
Nov 6, 2023
src/stirling/source_connectors/socket_tracer/protocols/common/event_parser.h
Outdated
Show resolved
Hide resolved
ddelnano
approved these changes
Nov 6, 2023
vihangm
pushed a commit
that referenced
this pull request
Nov 8, 2023
…commodate #1732 (#1761) Summary: Preemptively adapts the timestamp monotonicity change introduced in #1733 to the last stitcher api PR #1732, which modifies `ParseResult.frame_positions` to be an unordered `flat_hash_map`. This changes the order in which `GetTimestamp` is called because we are now iterating over an unordered map of streamIDs to positions when matching timestamps with the parsed frames in the [event_parser](https://github.com/pixie-io/pixie/blob/e6bfab707f1f4871f4b7b8ed53321ec9e7b5807d/src/stirling/source_connectors/socket_tracer/protocols/common/event_parser.h#L138C29-L138C36). Previously, we were always iterating over the `frame_position` with the oldest timestamp first, meaning that `prev_timestamp_` in the datastream buffer was set correctly. With `frame_positions` being an unordered map, we no longer have this guarantee. To address this, we move the monotonicity check to the `Head()` implementation of the datastream buffer and enforce increasing timestamps for the contiguous chunk returned by `Head()` only. Type of change: /kind bug Test Plan: Extended the data stream buffer test + existing targets. Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
benkilimnik
force-pushed
the
fix-cql-stitcher-4
branch
from
b008b52
to
6d97848
Compare
Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
benkilimnik
force-pushed
the
fix-cql-stitcher-4
branch
from
6d97848
to
30e7898
Compare
JamesMBartlett
approved these changes
Nov 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: Populates a map of streamIDs to deque of frames in
ParseFramesLoopinstead ofParseFrames. This should provide a small efficiency boost, as we won't have to loop over the frames twice. This PR relies on #1761 due to the way timestamps are updated usingParseResult.Related issues: #1375
Type of change: /kind cleanup
Test Plan: Updated parsing tests to use new interface