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

Turned off data integrity check #633

Merged
merged 9 commits into from
Jun 13, 2022
Merged

Turned off data integrity check #633

merged 9 commits into from
Jun 13, 2022

Conversation

avilevy18
Copy link
Contributor

@avilevy18 avilevy18 commented May 26, 2022

Turned off data integrity check when writing and reading logs from the filesystem for FluentBit

http://b/233109163

@avilevy18 avilevy18 force-pushed the storage-checksum-off branch 2 times, most recently from 48e400a to 373c711 Compare May 27, 2022 16:05
@avilevy18 avilevy18 force-pushed the storage-checksum-off branch from 373c711 to baca244 Compare May 27, 2022 16:26
@avilevy18 avilevy18 requested review from a team and igorpeshansky and removed request for a team June 3, 2022 20:15
Copy link
Member

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

One suggested change.

@@ -36,8 +36,8 @@ func (s Service) Component() Component {
// https://docs.fluentbit.io/manual/administration/buffering-and-storage#service-section-configuration
// storage.path is set by Fluent Bit systemd unit (e.g. /var/lib/google-cloud-ops-agent/fluent-bit/buffers).
"storage.sync": "normal",
// Enable the data integrity check when writing and reading data from the filesystem.
"storage.checksum": "on",
// Turned off due to empty spaces being considered as corrupted. Most users do not enforce checksum. See:b/233109163 for more info
Copy link
Member

Choose a reason for hiding this comment

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

How about:

Suggested change
// Turned off due to empty spaces being considered as corrupted. Most users do not enforce checksum. See:b/233109163 for more info
// Disable data integrity check to avoid spurious corruption errors due to empty spaces.
// Most users do not enforce checksum.

I don't believe there's a need to mention the bug in the code — git blame can trace it to this PR, which already references it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in: adf27bc

Copy link
Member

Choose a reason for hiding this comment

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

Note: Disable, not Disabled (because it describes what the option does, not what this PR did)…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback.

Changed in: d177711

Copy link
Contributor

@qingling128 qingling128 left a comment

Choose a reason for hiding this comment

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

LGTM

@avilevy18 avilevy18 added the kokoro:force-run Forces kokoro to run integration tests on a CL label Jun 13, 2022
@stackdriver-instrumentation-release stackdriver-instrumentation-release removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Jun 13, 2022
@avilevy18 avilevy18 merged commit 312285a into master Jun 13, 2022
@igorpeshansky igorpeshansky deleted the storage-checksum-off branch July 10, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants