-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[libbeat] Fix encoding and file offset issues in the disk queue #26484
Conversation
Pinging @elastic/agent (Team:Agent) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only partly understand the surrounding code, but the PR itself LGTM.
// A test to make sure serialization works correctly on multi-byte characters. | ||
func TestSerializeMultiByte(t *testing.T) { | ||
asciiOnly := "{\"name\": \"Momotaro\"}" | ||
multiBytes := "{\"name\": \"桃太郎\"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's create a table driven test and separate the 2 cases into separate tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -173,6 +173,9 @@ outerLoop: | |||
// This can only happen if the queue is being closed; abort. | |||
break | |||
} | |||
// We're creating a new segment file, set the initial bytes written | |||
// to the header size. | |||
curSegmentResponse.bytesWritten = wl.currentSegment.headerSize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a bug fix. Did we introduce the problem when moving to absolute adresses, or does it fix an issue we have had before? In the later case we might want to add a changelog entry (and maybe a separate PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this was just an oversight in the previous (absolute address) PR, so it hasn't hit a release.
(cherry picked from commit a3b447a)
* master: Osquerybeat: set the raw index name to supress the timestamp suffix (elastic#26545) [Heartbeat] add screenshots config to synthetics (elastic#26455) [Elastic Agent] Use http2 to connect to Fleet Server. (elastic#26474) Remove all docs about Beats central management (elastic#26399) update data.json for gcp billing (elastic#26506) Skip x-pack metricbeat tests (elastic#26537) [Elastic Agent] Fix issue with FLEET_CA not being used with Fleet Server in container (elastic#26529) Add changelog entry for elastic#26224 (elastic#26531) Add inttests for the x-pack/metricbeat on a PR/branches basis (elastic#26526) Suppress too many errors (elastic#26224) Fix master's linting issue (elastic#26517) [libbeat] Fix encoding and file offset issues in the disk queue (elastic#26484) Add log_group_name_prefix config option for aws-cloudwatch input (elastic#26187) Update shared-deduplication.asciidoc (elastic#26492) Add Recorded Future support to threatintel module (elastic#26481)
What does this PR do?
This PR fixes #26335 (incorrect decoding of multi-byte characters) by changing the disk queue serialization format to CBOR rather than JSON. It also fixes some file position calculations that weren't properly updated along with the schema change in #22970.
There are no user-visible changes in this PR other than the queue working correctly on new segment files; the underlying schema is altered, but files created by 7.13 or earlier will deserialize with JSON as before.
Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration filesCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.